Re: [PATCH] git-send-email: add ~/.authinfo parsing
Junio C Hamano writes: > I see a lot of rerolls on the credential helper front, but is there > anybody working on hooking send-email to the credential framework? Not answering the question, but git-remote-mediawiki supports the credential framework. It is written in perl, and the credential support is rather cleanly written and doesn't have dependencies on the wiki part, so the way to go for send-email is probably to libify the credential support in git-remote-mediawiki, and to use it in send-email. -- Matthieu Moy http://www-verimag.imag.fr/~moy/ -- 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: importing two different trees into a fresh git repo
On Tue, Feb 05, 2013 at 01:46:09PM -0800, Constantine A. Murenin wrote: > I've encountered two problems so far: > > 0. After initialising the repository, I was unable to `git checkout > --orphan Debian-6.0.4-nginx-1.0.12` -- presumably it doesn't work when > the repo is empty? This sounds like a bug or an artefact of > implementation. I presume this can be worked around by committing > into master instead, and then doing `git checkout -b > Debian-6.0.4-nginx-1.0.12`, and then force-fixing the master somehow > later on. What version of git are you using? Using both "-b" and "--orphan" from a non-existing branch used to be broken, but was fixed by abe1998 (git checkout -b: allow switching out of an unborn branch, 2012-01-30), which first appeared in git v1.7.9.2. > 1. After making a mistake on my first commit (my first commit into > OpenBSD-5.2-nginx-1.2.2 orphan branch ended up including a directory > from master by mistake), I am now unable to rebase and "fixup" the > changes -- `git rebase --interactive HEAD~2` doesn't work, which, from > one perspective, makes perfect sense (indeed there's no prior > revision), but, from another, it's not immediately obvious how to > quickly work around it. You cannot ask to rebase onto HEAD~2 because it does not exist (I'm assuming from your description that HEAD~1 is the root of your repository). But you can use the "--root" flag to ask git to rebase all the way down to the roots, like: git rebase -i --root However, note that older versions of git do not support using "--root" with "-i". The first usable version is v1.7.12. -Peff -- 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: git-svn problems with white-space in tag names
On Sun, Jan 27, 2013 at 3:12 PM, Hans-Juergen Euler wrote: > This seems to be a problem of the windows version. At least with its > complete severity. Installed git on Ubuntu in a virtual machine was > able to clone the subversion repos past the tag with the white-space > at the end. I am not sure but apparently this tag has not been > converted. > > The git repos I could copy from Ubuntu to windows. So far no problems > seen in this copy on windows. > The cause of the problem is that the tagname isn't a valid filename on Windows, which git requires it to be. -- 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: [RFC] test-lib.sh: No POSIXPERM for cygwin
On Sun, Jan 27, 2013 at 3:57 PM, Torsten Bögershausen wrote: > t0070 and t1301 fail when running the test suite under cygwin. > Skip the failing tests by unsetting POSIXPERM. > But is this the real reason? I thought Cygwin implemented POSIX permissions...? -- 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 v3 0/8] Hiding refs
On 02/05/2013 06:27 PM, Junio C Hamano wrote: > Michael Haggerty writes: >> I would again like to express my discomfort about this feature, which is >> already listed as "will merge to next". > > Do not take "will merge to next" too literally. One major purpose > of marking a topic as such is exactly to solicit comments like this > ;-) I take "will merge to next" pretty seriously, because I know how hard it is to get *my* patch series to this state :-) >> * I didn't see a response to Peff's convincing arguments that this >> should be a client-side feature rather than a server-side feature [1]. > > Uncluttering is not about a choice client should make. "delayed > advertisement" is an orthogonal issue and requires a larger protocol > update (it needs to make "git fetch" speak first instead of the > current protocol in which "upload-pack" speaks first). There seem to be a few issues mixed up in this topic. It is hard to reason about your patch series without understanding which scenarios and problems it is meant to address. First the problems that we might like to solve: Clutter: The typical user is subjected to much unneeded clutter in the form of references that he/she will likely never use. Bandwidth: Interactions with the remote repo (clone, fetch, etc) are slowed down by the large volume of unnecessary data. Provenance: Users mistakenly think that content originates with the repository owner whereas it in fact came from some other (perhaps untrusted) source. Now, what are some use-case scenarios in which these problems arise? As I understand it, there are a few: Scenario 1: Some providers junk up their users' repositories with content that is not created by the repository's owner and that the owner doesn't want to appear to vouch for (e.g., GitHub pull requests). These references might sometimes be useful to fetch, singly or in bulk. Scenario 2: Some systems junk up their users' repositories with additional references that are not interesting to most pullers (e.g., Gerrit activity markers) though they don't add questionable content. Scenario 3: Some repository owners might *themselves* want to push references to their repository but hide them from most users (e.g., Junio's topic branches) or make them completely hidden from the rest of the world (e.g., proprietary vs. open-source branches). In most of these cases, it would be desirable for at least some users to be able to fetch and/or push hidden content. A first weakness of your proposal is that even though the hidden refs are (optionally) fetchable, there is *no* way to discover them remotely or to bulk-download them; they would have to be retrieved one by one using out-of-band information. And if I understand correctly, there would be no way to push hidden references remotely (whether manually or from some automated process). Such processes would have to be local to the machine holding the repository. A second weakness of your proposal is that the repository owner would *anyway* need local access to the repo server or the help of the provider to implement reference hiding (since hidden references cannot be configured remotely). Who will choose what references to hide? Most likely each provider will pick a one-size-fits-all configuration and apply it to all of the repos that they manage. All users would be at the mercy of their provider to make wise choices and would not be able to override the choice via their client. A third weakness of your hidden references proposal is that it is schizophrenic: some references are hidden and undiscoverable, but their content can nevertheless be made fetchable if the user happens to know the SHA1. This is more complicated to understand and reason about than the rule "exactly the content that is referred to by published references is fetchable". What would be a better way? Providers could expose multiple views of the same repository; for example, one view with just the uncluttered content, and a second view that includes *all* fetchable references. Accessing the repository via the first view would give all of the benefits provided by your hidden reference proposal. Accessing it via the second view would allow the hidden references to be fetched (even in bulk) using purely git tools. The documentation for the second view could explain that it contains un-vetted content. But your proposal does not admit two-tiered access to a single repository. You only support one hidden reference configuration that is applied to all remote access [1]. See below for more ideas about implementing multiple views. >> * I didn't see a response to my worries that this feature could be >> abused [3]. > > You can choose not to advertise allow-tip-sha1-in-want capability; I > do not think it is making things worse than the status quo. Yes, if the feature is turned off then it is not worse than the status quo. But what if the feature is turned on? Actually,
Re: [PATCH v3 0/8] Hiding refs
On Tue, Feb 5, 2013 at 5:29 PM, Michael Haggerty wrote: > Hiderefs creates a "dark" corner of a remote git repo that can hold > arbitrary content that is impossible for anybody to discover but > nevertheless possible for anybody to download (if they know the name of > a hidden reference). In earlier versions of the patch series I believe > that it was possible to push to a hidden reference hierarchy, which made > it possible to upload dark content. The new version appears (from the > code) to prohibit adding references in a hidden hierarchy, which would > close the main loophole that I was worried about. But the documentation > and the unit tests only explicitly say that updates and deletes are > prohibited; nothing is said about adding references (unless "update" is > understood to include "add"). I think the true behavior should be > clarified and tested. > > I was worried that somehow this "dark" content could be used for > malicious purposes; for example, pushing compromised code then > convincing somebody to download it by SHA1 with the implicit argument > "it's safe since it comes directly from the project's official > repository". If it is indeed impossible to populate the dark namespace > remotely then I can't think of a way to exploit it. Or you can think hiderefs is the first step to addressing the initial ref advertisment problem. The series says hidden refs are to be fetched out of band, but that's not the only way. A new extension can be added to the protocol later to let the client explore this dark space. It's only truly dark for old clients. We could even shed some light to old clients by sending a dummy ref with some loud name like PLEASE_UPDATE_TO_LATEST_GIT_TO_FETCH_REMAINING_REFS (new clients silently drop this ref) -- 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] Verify Content-Type from smart HTTP servers
On 01/31/2013 11:09 PM, Junio C Hamano wrote: > > -static int http_request_reauth(const char *url, void *result, int target, > +static int http_request_reauth(const char *url, > +struct strbuf *type, > +void *result, int target, > int options) > { > - int ret = http_request(url, result, target, options); > + int ret = http_request(url, type, result, target, options); > if (ret != HTTP_REAUTH) > return ret; > - return http_request(url, result, target, options); > + return http_request(url, type, result, target, options); > } This needs something like diff --git a/http.c b/http.c index d868d8b..da43be3 100644 --- a/http.c +++ b/http.c @@ -860,6 +860,8 @@ static int http_request_reauth(const char *url, int ret = http_request(url, type, result, target, options); if (ret != HTTP_REAUTH) return ret; + if (type) + strbuf_reset(type); return http_request(url, type, result, target, options); } on top. Otherwise we get "text/plainapplication/x-git-receive-pack-advertisement" when doing HTTP auth. Thanks. > -int http_get_strbuf(const char *url, struct strbuf *result, int options) > +int http_get_strbuf(const char *url, > + struct strbuf *type, > + struct strbuf *result, int options) > { > - return http_request_reauth(url, result, HTTP_REQUEST_STRBUF, options); > + return http_request_reauth(url, type, result, > +HTTP_REQUEST_STRBUF, options); > } > > /* > @@ -878,7 +891,7 @@ static int http_get_file(const char *url, const char > *filename, int options) > goto cleanup; > } > > - ret = http_request_reauth(url, result, HTTP_REQUEST_FILE, options); > + ret = http_request_reauth(url, NULL, result, HTTP_REQUEST_FILE, > options); > fclose(result); > > if ((ret == HTTP_OK) && move_temp_to_file(tmpfile.buf, filename)) > @@ -904,7 +917,7 @@ int http_fetch_ref(const char *base, struct ref *ref) > int ret = -1; > > url = quote_ref_url(base, ref->name); > - if (http_get_strbuf(url, &buffer, HTTP_NO_CACHE) == HTTP_OK) { > + if (http_get_strbuf(url, NULL, &buffer, HTTP_NO_CACHE) == HTTP_OK) { > strbuf_rtrim(&buffer); > if (buffer.len == 40) > ret = get_sha1_hex(buffer.buf, ref->old_sha1); > @@ -997,7 +1010,7 @@ int http_get_info_packs(const char *base_url, struct > packed_git **packs_head) > strbuf_addstr(&buf, "objects/info/packs"); > url = strbuf_detach(&buf, NULL); > > - ret = http_get_strbuf(url, &buf, HTTP_NO_CACHE); > + ret = http_get_strbuf(url, NULL, &buf, HTTP_NO_CACHE); > if (ret != HTTP_OK) > goto cleanup; > > diff --git a/http.h b/http.h > index 0a80d30..25d1931 100644 > --- a/http.h > +++ b/http.h > @@ -132,7 +132,7 @@ extern char *get_remote_object_url(const char *url, const > char *hex, > * > * If the result pointer is NULL, a HTTP HEAD request is made instead of GET. > */ > -int http_get_strbuf(const char *url, struct strbuf *result, int options); > +int http_get_strbuf(const char *url, struct strbuf *content_type, struct > strbuf *result, int options); > > /* > * Prints an error message using error() containing url and curl_errorstr, > diff --git a/remote-curl.c b/remote-curl.c > index 9a8b123..e6f3b63 100644 > --- a/remote-curl.c > +++ b/remote-curl.c > @@ -92,6 +92,8 @@ static void free_discovery(struct discovery *d) > > static struct discovery* discover_refs(const char *service) > { > + struct strbuf exp = STRBUF_INIT; > + struct strbuf type = STRBUF_INIT; > struct strbuf buffer = STRBUF_INIT; > struct discovery *last = last_discovery; > char *refs_url; > @@ -113,7 +115,7 @@ static struct discovery* discover_refs(const char > *service) > } > refs_url = strbuf_detach(&buffer, NULL); > > - http_ret = http_get_strbuf(refs_url, &buffer, HTTP_NO_CACHE); > + http_ret = http_get_strbuf(refs_url, &type, &buffer, HTTP_NO_CACHE); > switch (http_ret) { > case HTTP_OK: > break; > @@ -133,16 +135,19 @@ static struct discovery* discover_refs(const char > *service) > last->buf = last->buf_alloc; > > if (maybe_smart && 5 <= last->len && last->buf[4] == '#') { > - /* smart HTTP response; validate that the service > + /* > + * smart HTTP response; validate that the service >* pkt-line matches our request. >*/ > - struct strbuf exp = STRBUF_INIT; > - > + strbuf_addf(&exp, "application/x-%s-advertisement", service); > + if (strbuf_cmp(&exp, &type)) > + die("invalid content-type %s", type.buf); > if (packet_get_line(&buffer, &last->buf, &last->len) <= 0) >
Re: [PATCH] Verify Content-Type from smart HTTP servers
On Wed, Feb 06, 2013 at 11:24:41AM +0100, Michael Schubert wrote: > On 01/31/2013 11:09 PM, Junio C Hamano wrote: > > > > > -static int http_request_reauth(const char *url, void *result, int target, > > +static int http_request_reauth(const char *url, > > + struct strbuf *type, > > + void *result, int target, > >int options) > > { > > - int ret = http_request(url, result, target, options); > > + int ret = http_request(url, type, result, target, options); > > if (ret != HTTP_REAUTH) > > return ret; > > - return http_request(url, result, target, options); > > + return http_request(url, type, result, target, options); > > } > > This needs something like > > diff --git a/http.c b/http.c > index d868d8b..da43be3 100644 > --- a/http.c > +++ b/http.c > @@ -860,6 +860,8 @@ static int http_request_reauth(const char *url, > int ret = http_request(url, type, result, target, options); > if (ret != HTTP_REAUTH) > return ret; > + if (type) > + strbuf_reset(type); > return http_request(url, type, result, target, options); > } > > on top. Otherwise we get > > "text/plainapplication/x-git-receive-pack-advertisement" > > when doing HTTP auth. Good catch. It probably makes sense to put it in http_request, so that we also protect against any existing cruft from the callers of http_get_*, like: -- >8 -- Subject: [PATCH] http_request: reset "type" strbuf before adding Callers may pass us a strbuf which we use to record the content-type of the response. However, we simply appended to it rather than overwriting its contents, meaning that cruft in the strbuf gave us a bogus type. E.g., the multiple requests triggered by http_request could yield a type like "text/plainapplication/x-git-receive-pack-advertisement". Reported-by: Michael Schubert Signed-off-by: Jeff King --- Is it worth having a strbuf_set* family of functions to match the strbuf_add*? We seem to have these sorts of errors with strbuf from time to time, and I wonder if that would make it easier (and more readable) to do the right thing. http.c | 1 + 1 file changed, 1 insertion(+) diff --git a/http.c b/http.c index d868d8b..d9d1aad 100644 --- a/http.c +++ b/http.c @@ -841,6 +841,7 @@ static int http_request(const char *url, struct strbuf *type, if (type) { char *t; + strbuf_reset(type); curl_easy_getinfo(slot->curl, CURLINFO_CONTENT_TYPE, &t); if (t) strbuf_addstr(type, t); -- 1.8.1.2.11.g1a2f572 -- 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 v2 2/2] i18n: mark OPTION_NUMBER (-NUM) for translation
On Wed, Feb 6, 2013 at 11:35 AM, Junio C Hamano wrote: >> How about this since [PATCH v3]: >> >> diff --git a/utf8.c b/utf8.c >> index 52dbd..b893a 100644 >> --- a/utf8.c >> +++ b/utf8.c >> @@ -443,8 +443,11 @@ int utf8_fprintf(FILE *stream, const char *format, ...) >> strbuf_vaddf(&buf, format, arg); >> va_end (arg); >> >> - fputs(buf.buf, stream); >> - columns = utf8_strwidth(buf.buf); >> + columns = fputs(buf.buf, stream); >> + /* If no error occurs, and really write something (columns > 0), >> +* calculate really columns width with utf8_strwidth. */ >> + if (columns > 0) >> + columns = utf8_strwidth(buf.buf); >> strbuf_release(&buf); >> return columns; >> } > > The above bugfix does not address my original concern about > the name, though. How about utf8_fwprintf? wprintf() deals with wide characters and returns the number of wide characters, I think the name fits. And we could just drop utf8_ and use the existing name, because we don't use wchar_t* anyway. -- 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: Adding Missing Tags to a Repository
Neil venit, vidit, dixit 06.02.2013 05:45: > Hi everyone, > > A while back I did a svn-to-git migration for my team. Our subversion > repository had about 30K+ commits, 100+ branches, 2K+ tags, all made > over a 20+ year period. I was doing the migration using git-svn, and > my big problem was the tags. git-svn seemed to want to traverse the > entire history of each tag, which was taking a long time. Because time > and resources were limited, I ended up just migrating the branches and > trunk, with the idea that I would handle the tags at a later date. My > original plan to do that was to crawl the subversion log, find where > the tags were made, and apply a git tag to the commit that was the > source of the tag. This was a bad idea. > > I've found that over the years, people have made tags that are only > subdirectories of the source tree, made tags off of other tags, and > committed to tags. The latter is the biggest problem, since those > commits don't seem to be stored in the git repository because they > never appeared in the branches/trunk. I think it's fair to say that svn encourages the abuse of tags ;) I've cleaned up other people's mis-tags myself, and it's no fun. If the tree layout is messed up then git-svn will often create a new root commit, so that you have to stitch the history yourself. In the case of "correct" tags, you're often better of converting the tag creating commit into an annotated tag, provided that the tree is unchanged. Tags with tree changes are really branches, usually maintenance branches after a proper tag. > So, I'm wondering what my options are to bring back this history. One > idea is to somehow resume the git-svn download, but changing it to > also scan tags (it sounds like it should be possible, but I haven't > tried it yet). Or maybe there's some other tool that will more quickly > clone the repository including tags, and then I can somehow splice the > tags back into the repository we're already using? > > Any ideas or suggestions? > > Thanks! > If you add a git-svn refspec to your config and rerun git-svn fetch it might rescan tags, although you may have to poke around to make it rescan from revision 1. Michael -- 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 v3 3/8] upload/receive-pack: allow hiding ref hierarchies
On Tue, Feb 05, 2013 at 07:45:01AM -0800, Junio C Hamano wrote: > > In the earlier review, I mentioned making this per-service, but I see > > that is not the case here. Do you have an argument against doing so? > > Perhaps then I misunderstood your intention. By reminding me of the > receive-pack side, I thought you were hinting to unify these two > into one, which I did. There is no argument against it. What I meant was that there should be transfer.hiderefs, and an individual {receive,uploadpack}.hiderefs, similar to the way we have transfer.unpacklimit. That makes the easy case (hiding the refs completely) easy, but leaves the flexibility for more. Like this: diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c index a8248d9..131c163 100644 --- a/builtin/receive-pack.c +++ b/builtin/receive-pack.c @@ -59,7 +59,7 @@ static int receive_pack_config(const char *var, const char *value, void *cb) static int receive_pack_config(const char *var, const char *value, void *cb) { - int status = parse_hide_refs_config(var, value, cb); + int status = parse_hide_refs_config(var, value, "receive"); if (status) return status; diff --git a/refs.c b/refs.c index e3574ca..9bfea58 100644 --- a/refs.c +++ b/refs.c @@ -2560,9 +2560,13 @@ int parse_hide_refs_config(const char *var, const char *value, void *unused) static struct string_list *hide_refs; -int parse_hide_refs_config(const char *var, const char *value, void *unused) +int parse_hide_refs_config(const char *var, const char *value, void *vsection) { - if (!strcmp("transfer.hiderefs", var)) { + const char *section = vsection; + + if (!strcmp("transfer.hiderefs", var) || + (!prefixcmp(var, section) && +!strcmp(var + strlen(section), ".hiderefs"))) { char *ref; int len; diff --git a/upload-pack.c b/upload-pack.c index 37977e2..c0390af 100644 --- a/upload-pack.c +++ b/upload-pack.c @@ -794,7 +794,7 @@ static int upload_pack_config(const char *var, const char *value, void *unused) { if (!strcmp("uploadpack.allowtipsha1inwant", var)) allow_tip_sha1_in_want = git_config_bool(var, value); - return parse_hide_refs_config(var, value, unused); + return parse_hide_refs_config(var, value, "uploadpack"); } int main(int argc, char **argv) As an aside, I wonder if there is any point to the void pointer parameter of parse_hide_refs_config. It is not used as a git_config callback anywhere. > > And I > > have not seen complaints about the current system. > > Immediately after I added github to the set of places I push into, > which I think is long before you joined GitHub, I noticed that _my_ > repository gets contaminated by second rate commits called pull > requests, and I may even have complained, but most likely I didn't, > as I could easily tell that, even though I know it is _not_ the only > way, nor even the best way [*1*], to implement the GitHub's pull > request workflow, I perfectly well understood that it would be the > most expedient way for GitHub folks to implement this feature. > > I think you should take lack of complaints with a huge grain of > salt. It does not suggest much. Sure, I do not pretend that nobody cares. But it is certainly not a pressing issue, or there would probably be more outcry. And we must also weigh it against the silent majority that are perfectly happy with the status quo, that lets them fetch refs/pull/* as any other ref. In your case, I really think the problem is less that you have a problem with PR refs in the repository, and more that you do not care about the pull request feature at all. To you it is useless noise, both in the repo and on the web. Your arguments about provenance could apply equally well to PRs accessible via the web interface. I think the refs/ clutter is only an issue if you want to do mirroring, and then you have an obvious conflict: did the fetcher want to mirror everything, including refs/pull, or do they consider that to be clutter? Only the client knows, which is why I think refspecs are the right place to deal with clutter (the fact that we cannot say "everything except refs/pull/*" is a weakness in our refspecs). > *1* From the ownership point of view, objects that are only > reachable from these refs/pull/* refs do *not* belong to the > requestee, until the requestee chooses to accept the changes. > > A malicious requestor can fork your repository, add an objectionable > blob to it, and throw a pull request at you. GitHub shows that the > blob now belongs to your repository, so the requestor turns around > and file a DMCA takedown complaint against your repository. A > clueful judge would then agree with the complaint after running a > "clone --mirror" and seeing the blob in your repository. Oops? I don't think this is a problem in practice. DMCA notices do not go to the repository owner; they go to GitHub. And as far as I
Why is ident_is_sufficient different on Windows?
Hi there, while trying to understand which parts of the author & committer identity are mandatory (name, email, or both), I ended up in ident.c, looking at ident_is_sufficient(), and to my surprise discovered that this seems to differ between Windows (were both are mandatory) and everyone else: static int ident_is_sufficient(int user_ident_explicitly_given) { #ifndef WINDOWS return (user_ident_explicitly_given & IDENT_MAIL_GIVEN); #else return (user_ident_explicitly_given == IDENT_ALL_GIVEN); #endif } According to git blame, this was introduced here: commit 5aeb3a3a838b2cb03d250f3376cf9c41f4d4608e Author: Junio C Hamano Date: Sun Jan 17 13:54:28 2010 -0800 user_ident_sufficiently_given(): refactor the logic to be usable from elsewhere The commit message sounds as if this was only a refactoring, but the patch to me look as if it changes behaviour, too. Of course this could very well be false, say due to code elsewhere that already caused Windows to behave differently; I wouldn't know. Still, I wonder: Why does this difference exist? Cheers, Max -- 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] git-send-email: add ~/.authinfo parsing
On Wed, Feb 06 2013, Junio C Hamano wrote: > I see a lot of rerolls on the credential helper front, but is there > anybody working on hooking send-email to the credential framework? I assumed someone had, but if not I can take a stab at it. I'm not sure however how should I map server, server-port, and user to credential key-value pairs. I'm leaning towards protocol=smtp host=: user= and than netrc/authinfo helper splitting host to host name and port number, unless port is not in host in which case protocol is assumed as port. -- Best regards, _ _ .o. | Liege of Serenely Enlightened Majesty of o' \,=./ `o ..o | Computer Science, Michał “mina86” Nazarewicz(o o) ooo +--ooO--(_)--Ooo-- pgpyBEG11AUhI.pgp Description: PGP signature
Re: [PATCH] git-send-email: add ~/.authinfo parsing
On Wed, 06 Feb 2013 14:26:46 +0100 Michal Nazarewicz wrote: MN> On Wed, Feb 06 2013, Junio C Hamano wrote: >> I see a lot of rerolls on the credential helper front, but is there >> anybody working on hooking send-email to the credential framework? MN> I assumed someone had, but if not I can take a stab at it. I'm not sure MN> however how should I map server, server-port, and user to credential MN> key-value pairs. I'm leaning towards MN> protocol=smtp MN> host=: MN> user= MN> and than netrc/authinfo helper splitting host to host name and port MN> number, unless port is not in host in which case protocol is assumed as MN> port. That would work (with my PATCHv6 of the netrc credential helper) as follows: 1) just host host=H maps to machine H login Y password Z 2) host + protocol smtp host=H protocol=smtp maps to any of: machine H port smtp login Y password Z machine H protocol smtp login Y password Z 3) host:port + protocol smtp host=H:25 protocol=smtp maps to any of: machine H port 25 protocol smtp login Y password Z machine H:25 port smtp login Y password Z machine H:25 protocol smtp login Y password Z That's my understanding of what we discussed with Peff and Junio about token mapping. Note we don't split the input host, but instead say "if token 'port' is numeric, append it to the host token" on the netrc side. Does that sound reasonable? If yes, I can add it to the testing Makefile for the netrc credential helper, to make sure it's clearly stated and tested. Ted -- 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] git-send-email: add ~/.authinfo parsing
On Wed, 06 Feb 2013 09:11:17 +0100 Matthieu Moy wrote: MM> Junio C Hamano writes: >> I see a lot of rerolls on the credential helper front, but is there >> anybody working on hooking send-email to the credential framework? MM> Not answering the question, but git-remote-mediawiki supports the MM> credential framework. It is written in perl, and the credential support MM> is rather cleanly written and doesn't have dependencies on the wiki MM> part, so the way to go for send-email is probably to libify the MM> credential support in git-remote-mediawiki, and to use it in send-email. I looked and that's indeed very useful. If it's put in a library, I'd use credential_read() and credential_write() in my netrc credential helper. But I would formalize it a little more about the token names and output, and I wouldn't necessarily die() on error. Maybe this can be merged with the netrc credential helper's read_credential_data_from_stdin() and print_credential_data()? Let me know if you'd like me to libify this... I'm happy to leave it to Matthieu or Michal, or anyone else interested. Ted -- 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: Bug in "git log --graph -p -m" (version 1.7.7.6)
> From: Matthieu Moy > > In any case, I can't reproduce with 1.8.1.2.526.gf51a757: I don't get > undless output. On the other hand, I get a slightly misformatted output: > > * commit a393ed598e9fb11436f85bd58f1a38c82f2cadb7 (from > 2c1e6a36f4b712e914fac994463da7d0fdb2bc6d) > |\ Merge: 2c1e6a3 33e70e7 > | | Author: Matthieu Moy > | | Date: Tue Feb 5 22:05:33 2013 +0100 > | | > | | Commit S > | | > | | diff --git a/file b/file > | | index 6bb4d3e..afd2e75 100644 > | | --- a/file > | | +++ b/file > | | @@ -1,4 +1,5 @@ > | | 1 > | | 1a > | | 2 > | | +2a > | | 3 > | | > commit a393ed598e9fb11436f85bd58f1a38c82f2cadb7 (from > 33e70e70c0173d634826b998bdc304f93c0966b8) > | | Merge: 2c1e6a3 33e70e7 > | | Author: Matthieu Moy > | | Date: Tue Feb 5 22:05:33 2013 +0100 > > The second "commit" line (diff with second parent) doesn't have the > "| |" prefix, I don't think this is intentional. The second "commit" line should start with "| * ": > | | > | * commit a393ed598e9fb11436f85bd58f1a38c82f2cadb7 (from > 33e70e70c0173d634826b998bdc304f93c0966b8) > | | Merge: 2c1e6a3 33e70e7 > | | Author: Matthieu Moy > | | Date: Tue Feb 5 22:05:33 2013 +0100 Dale -- 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
[RFC/PATCH 0/4] textconv for show and grep
This min series aims at completing the textconv support of user facing commands. It is RFC for lack of documentation and tests, to check whether we really want to go in that direction (I do). 1/4 covers the missing textconv support in the "blob case" of "git show", which should be (and then is) analogous to "cat-file" and "diff". 2/4 remedies an oddity of "git cat-file --textconv", which errored out when there is no textconv filter rather than giving an unfiltered blob (like every other textconv aware command). 3/4 implements "--textconv" for "git grep" sans the blob case; the code is all Jeff's. 4/4 adds blob support to 3/4 (the "rev:path" case). 3 and 4 can be squashed, of course. Now I'm quite a happy differ/shower/grepper with my latin1 and OO files ;) Jeff King (1): grep: allow to use textconv filters Michael J Gruber (3): show: obey --textconv for blobs cat-file: do not die on --textconv without textconv filters grep: obey --textconv for the case rev:path builtin/cat-file.c | 9 ++-- builtin/grep.c | 13 +++--- builtin/log.c| 24 +-- grep.c | 100 +-- grep.h | 1 + object.c | 26 --- object.h | 2 + t/t8007-cat-file-textconv.sh | 20 +++-- 8 files changed, 148 insertions(+), 47 deletions(-) -- 1.8.1.2.752.g32d147e -- 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
[RFC/PATCH 1/4] show: obey --textconv for blobs
Currently, "diff" and "cat-file" for blobs obey "--textconv" options (with the former defaulting to "--textconv" and the latter to "--no-textconv") whereas "show" does not obey this option, even though it takes diff options. Make "show" on blobs behave like "diff", i.e. obey "--textconv" by default and "--no-textconv" when given. Signed-off-by: Michael J Gruber --- builtin/log.c | 24 +--- 1 file changed, 21 insertions(+), 3 deletions(-) diff --git a/builtin/log.c b/builtin/log.c index 8f0b2e8..f83870d 100644 --- a/builtin/log.c +++ b/builtin/log.c @@ -402,10 +402,28 @@ static void show_tagger(char *buf, int len, struct rev_info *rev) strbuf_release(&out); } -static int show_blob_object(const unsigned char *sha1, struct rev_info *rev) +static int show_blob_object(const unsigned char *sha1, struct rev_info *rev, const char *obj_name) { + unsigned char sha1c[20]; + struct object_context obj_context; + char *buf; + unsigned long size; + fflush(stdout); - return stream_blob_to_fd(1, sha1, NULL, 0); + if (!DIFF_OPT_TST(&rev->diffopt, ALLOW_TEXTCONV)) + return stream_blob_to_fd(1, sha1, NULL, 0); + + if (get_sha1_with_context(obj_name, 0, sha1c, &obj_context)) + die("Not a valid object name %s", obj_name); + if (!obj_context.path[0] || + !textconv_object(obj_context.path, obj_context.mode, sha1c, 1, &buf, &size)) + return stream_blob_to_fd(1, sha1, NULL, 0); + + if (!buf) + die("git show %s: bad file", obj_name); + + write_or_die(1, buf, size); + return 0; } static int show_tag_object(const unsigned char *sha1, struct rev_info *rev) @@ -491,7 +509,7 @@ int cmd_show(int argc, const char **argv, const char *prefix) const char *name = objects[i].name; switch (o->type) { case OBJ_BLOB: - ret = show_blob_object(o->sha1, NULL); + ret = show_blob_object(o->sha1, &rev, name); break; case OBJ_TAG: { struct tag *t = (struct tag *)o; -- 1.8.1.2.752.g32d147e -- 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
[RFC/PATCH 2/4] cat-file: do not die on --textconv without textconv filters
When a command is supposed to use textconv filters (by default or with "--textconv") and none are configured then the blob is output without conversion; the only exception to this rule is "cat-file --textconv". Make it behave like the rest of textconv aware commands. Signed-off-by: Michael J Gruber --- builtin/cat-file.c | 9 + t/t8007-cat-file-textconv.sh | 20 +--- 2 files changed, 10 insertions(+), 19 deletions(-) diff --git a/builtin/cat-file.c b/builtin/cat-file.c index 00528dd..6912dc2 100644 --- a/builtin/cat-file.c +++ b/builtin/cat-file.c @@ -146,10 +146,11 @@ static int cat_one_file(int opt, const char *exp_type, const char *obj_name) die("git cat-file --textconv %s: must be ", obj_name); - if (!textconv_object(obj_context.path, obj_context.mode, sha1, 1, &buf, &size)) - die("git cat-file --textconv: unable to run textconv on %s", - obj_name); - break; + if (textconv_object(obj_context.path, obj_context.mode, sha1, 1, &buf, &size)) + break; + + /* otherwise expect a blob */ + exp_type = "blob"; case 0: if (type_from_string(exp_type) == OBJ_BLOB) { diff --git a/t/t8007-cat-file-textconv.sh b/t/t8007-cat-file-textconv.sh index 78a0085..83c6636 100755 --- a/t/t8007-cat-file-textconv.sh +++ b/t/t8007-cat-file-textconv.sh @@ -22,11 +22,11 @@ test_expect_success 'setup ' ' ' cat >expectedresult && test_cmp expected result ' @@ -36,10 +36,6 @@ test_expect_success 'setup textconv filters' ' git config diff.test.cachetextconv false ' -cat >expected expected && git cat-file blob :symlink.bin >result && - printf "%s" "one.bin" >expected test_cmp expected result ' test_expect_success SYMLINKS 'cat-file --textconv on index (symlink)' ' - ! git cat-file --textconv :symlink.bin 2>result && - cat >expected <<\EOF && -fatal: git cat-file --textconv: unable to run textconv on :symlink.bin -EOF + git cat-file --textconv :symlink.bin >result && test_cmp expected result ' test_expect_success SYMLINKS 'cat-file --textconv on HEAD (symlink)' ' - ! git cat-file --textconv HEAD:symlink.bin 2>result && - cat >expected
[RFC/PATCH 3/4] grep: allow to use textconv filters
From: Jeff King Recently and not so recently, we made sure that log/grep type operations use textconv filters when a userfacing diff would do the same: ef90ab6 (pickaxe: use textconv for -S counting, 2012-10-28) b1c2f57 (diff_grep: use textconv buffers for add/deleted files, 2012-10-28) 0508fe5 (combine-diff: respect textconv attributes, 2011-05-23) "git grep" currently does not use textconv filters at all, that is neither for displaying the match and context nor for the actual grepping. Introduce an option "--textconv" which makes git grep use any configured textconv filters for grepping and output purposes. It is off by default. Signed-off-by: Michael J Gruber --- builtin/grep.c | 2 ++ grep.c | 100 + grep.h | 1 + 3 files changed, 89 insertions(+), 14 deletions(-) diff --git a/builtin/grep.c b/builtin/grep.c index 8025964..915c8ef 100644 --- a/builtin/grep.c +++ b/builtin/grep.c @@ -659,6 +659,8 @@ int cmd_grep(int argc, const char **argv, const char *prefix) OPT_SET_INT('I', NULL, &opt.binary, N_("don't match patterns in binary files"), GREP_BINARY_NOMATCH), + OPT_BOOL(0, "textconv", &opt.allow_textconv, +N_("process binary files with textconv filters")), { OPTION_INTEGER, 0, "max-depth", &opt.max_depth, N_("depth"), N_("descend at most levels"), PARSE_OPT_NONEG, NULL, 1 }, diff --git a/grep.c b/grep.c index 4bd1b8b..3880d64 100644 --- a/grep.c +++ b/grep.c @@ -2,6 +2,8 @@ #include "grep.h" #include "userdiff.h" #include "xdiff-interface.h" +#include "diff.h" +#include "diffcore.h" static int grep_source_load(struct grep_source *gs); static int grep_source_is_binary(struct grep_source *gs); @@ -1321,6 +1323,58 @@ static void std_output(struct grep_opt *opt, const void *buf, size_t size) fwrite(buf, size, 1, stdout); } +static int fill_textconv_grep(struct userdiff_driver *driver, + struct grep_source *gs) +{ + struct diff_filespec *df; + char *buf; + size_t size; + + if (!driver || !driver->textconv) + return grep_source_load(gs); + + /* +* The textconv interface is intimately tied to diff_filespecs, so we +* have to pretend to be one. If we could unify the grep_source +* and diff_filespec structs, this mess could just go away. +*/ + df = alloc_filespec(gs->path); + switch (gs->type) { + case GREP_SOURCE_SHA1: + fill_filespec(df, gs->identifier, 1, 0100644); + break; + case GREP_SOURCE_FILE: + fill_filespec(df, null_sha1, 0, 0100644); + break; + default: + die("BUG: attempt to textconv something without a path?"); + } + + /* +* fill_textconv is not remotely thread-safe; it may load objects +* behind the scenes, and it modifies the global diff tempfile +* structure. +*/ + grep_read_lock(); + size = fill_textconv(driver, df, &buf); + grep_read_unlock(); + free_filespec(df); + + /* +* The normal fill_textconv usage by the diff machinery would just keep +* the textconv'd buf separate from the diff_filespec. But much of the +* grep code passes around a grep_source and assumes that its "buf" +* pointer is the beginning of the thing we are searching. So let's +* install our textconv'd version into the grep_source, taking care not +* to leak any existing buffer. +*/ + grep_source_clear_data(gs); + gs->buf = buf; + gs->size = size; + + return 0; +} + static int grep_source_1(struct grep_opt *opt, struct grep_source *gs, int collect_hits) { char *bol; @@ -1331,6 +1385,7 @@ static int grep_source_1(struct grep_opt *opt, struct grep_source *gs, int colle unsigned count = 0; int try_lookahead = 0; int show_function = 0; + struct userdiff_driver *textconv = NULL; enum grep_context ctx = GREP_CONTEXT_HEAD; xdemitconf_t xecfg; @@ -1352,19 +1407,36 @@ static int grep_source_1(struct grep_opt *opt, struct grep_source *gs, int colle } opt->last_shown = 0; - switch (opt->binary) { - case GREP_BINARY_DEFAULT: - if (grep_source_is_binary(gs)) - binary_match_only = 1; - break; - case GREP_BINARY_NOMATCH: - if (grep_source_is_binary(gs)) - return 0; /* Assume unmatch */ - break; - case GREP_BINARY_TEXT: - break; - default: - die("bug: unknown binary handling mode"); + if (opt->allow_textconv) { + grep_source_load_driver(gs); + /* +
[RFC/PATCH 4/4] grep: obey --textconv for the case rev:path
Make "grep" obey the "--textconv" option also for the object case, i.e. when used with an argument "rev:path". Signed-off-by: Michael J Gruber --- builtin/grep.c | 11 ++- object.c | 26 -- object.h | 2 ++ 3 files changed, 28 insertions(+), 11 deletions(-) diff --git a/builtin/grep.c b/builtin/grep.c index 915c8ef..0f3c4db 100644 --- a/builtin/grep.c +++ b/builtin/grep.c @@ -458,10 +458,10 @@ static int grep_tree(struct grep_opt *opt, const struct pathspec *pathspec, } static int grep_object(struct grep_opt *opt, const struct pathspec *pathspec, - struct object *obj, const char *name) + struct object *obj, const char *name, struct object_context *oc) { if (obj->type == OBJ_BLOB) - return grep_sha1(opt, obj->sha1, name, 0, NULL); + return grep_sha1(opt, obj->sha1, name, 0, oc ? oc->path : NULL); if (obj->type == OBJ_COMMIT || obj->type == OBJ_TREE) { struct tree_desc tree; void *data; @@ -503,7 +503,7 @@ static int grep_objects(struct grep_opt *opt, const struct pathspec *pathspec, for (i = 0; i < nr; i++) { struct object *real_obj; real_obj = deref_tag(list->objects[i].item, NULL, 0); - if (grep_object(opt, pathspec, real_obj, list->objects[i].name)) { + if (grep_object(opt, pathspec, real_obj, list->objects[i].name, list->objects[i].context)) { hit = 1; if (opt->status_only) break; @@ -820,14 +820,15 @@ int cmd_grep(int argc, const char **argv, const char *prefix) for (i = 0; i < argc; i++) { const char *arg = argv[i]; unsigned char sha1[20]; + struct object_context oc; /* Is it a rev? */ - if (!get_sha1(arg, sha1)) { + if (!get_sha1_with_context(arg, 0, sha1, &oc)) { struct object *object = parse_object(sha1); if (!object) die(_("bad object %s"), arg); if (!seen_dashdash) verify_non_filename(prefix, arg); - add_object_array(object, arg, &list); + add_object_array_with_context(object, arg, &list, xmemdupz(&oc, sizeof(struct object_context))); continue; } if (!strcmp(arg, "--")) { diff --git a/object.c b/object.c index 4af3451..1b796c7 100644 --- a/object.c +++ b/object.c @@ -245,12 +245,7 @@ int object_list_contains(struct object_list *list, struct object *obj) return 0; } -void add_object_array(struct object *obj, const char *name, struct object_array *array) -{ - add_object_array_with_mode(obj, name, array, S_IFINVALID); -} - -void add_object_array_with_mode(struct object *obj, const char *name, struct object_array *array, unsigned mode) +static void add_object_array_with_mode_context(struct object *obj, const char *name, struct object_array *array, unsigned mode, struct object_context *context) { unsigned nr = array->nr; unsigned alloc = array->alloc; @@ -265,9 +260,28 @@ void add_object_array_with_mode(struct object *obj, const char *name, struct obj objects[nr].item = obj; objects[nr].name = name; objects[nr].mode = mode; + objects[nr].context = context; array->nr = ++nr; } +void add_object_array(struct object *obj, const char *name, struct object_array *array) +{ + add_object_array_with_mode(obj, name, array, S_IFINVALID); +} + +void add_object_array_with_mode(struct object *obj, const char *name, struct object_array *array, unsigned mode) +{ + add_object_array_with_mode_context(obj, name, array, mode, NULL); +} + +void add_object_array_with_context(struct object *obj, const char *name, struct object_array *array, struct object_context *context) +{ + if (context) + add_object_array_with_mode_context(obj, name, array, context->mode, context); + else + add_object_array_with_mode_context(obj, name, array, S_IFINVALID, context); +} + void object_array_remove_duplicates(struct object_array *array) { unsigned int ref, src, dst; diff --git a/object.h b/object.h index 6a97b6b..a11d719 100644 --- a/object.h +++ b/object.h @@ -13,6 +13,7 @@ struct object_array { struct object *item; const char *name; unsigned mode; + struct object_context *context; } *objects; }; @@ -74,6 +75,7 @@ int object_list_contains(struct object_list *list, struct object *obj); /* Object array handling .. */ void add_object_array(struct object *obj, const char *name, struct object_array *array); void add_object_array_with_mode(struct object *obj, const char *name,
CodingGuidelines Perl amendment (was: [PATCH 1/3] Add contrib/credentials/netrc with GPG support)
On Mon, 04 Feb 2013 15:10:45 -0800 Junio C Hamano wrote: JCH> Ted Zlatanov writes: JCH> I thought that we tend to avoid Emacs/Vim formatting cruft left in JCH> the file. Do we have any in existing file outside contrib/? >> >> No, but it's a nice way to express the settings so no one is guessing >> what the project prefers. At least for me it's not an issue anymore, >> since I understand your criteria better now, so let me know if you want >> me to express it in the CodingGuidelines, in a dir-locals.el file, or >> somewhere else. JCH> Historically we treated this from CodingGuidelines a sufficient JCH> clue: JCH> As for more concrete guidelines, just imitate the existing code JCH> (this is a good guideline, no matter which project you are JCH> contributing to). It is always preferable to match the _local_ JCH> convention. New code added to git suite is expected to match JCH> the overall style of existing code. Modifications to existing JCH> code is expected to match the style the surrounding code already JCH> uses (even if it doesn't match the overall style of existing code). JCH> but over time people wanted more specific guidelines and added JCH> language specific style guides there. We have sections that cover JCH> C, shell and Python, and I do not think adding Perl would not hurt. The following is how I have interpreted the Perl guidelines. I hope it's OK to include Emacs-specific settings; they make it much easier to reindent code to be acceptable. I will submit as a patch if you think this is reasonable at all. The org-mode markers around the code are just a suggestion. For Perl 5 programs: - Most of the C guidelines above apply. - We try to support Perl 5.8 and later ("use Perl 5.008"). - use strict and use warnings are strongly preferred. - As in C (see above), we avoid using braces unnecessarily (but Perl forces braces around if/unless/else/foreach blocks, so this is not always possible). - Don't abuse statement modifiers (unless $youmust). - We try to avoid assignments inside if(). - Learn and use Git.pm if you need that functionality. - For Emacs, it's useful to put the following in GIT_CHECKOUT/.dir-locals.el, assuming you use cperl-mode: #+begin_src lisp ((nil . ((indent-tabs-mode . t) (tab-width . 8) (fill-column . 80))) (cperl-mode . ((cperl-indent-level . 8) (cperl-extra-newline-before-brace . nil) (cperl-merge-trailing-else . t #+end_src -- 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] git-send-email: add ~/.authinfo parsing
Ted Zlatanov writes: > MM> [...] so the way to go for send-email is probably to libify the > MM> credential support in git-remote-mediawiki, and to use it in send-email. > > I looked and that's indeed very useful. If it's put in a library, I'd > use credential_read() and credential_write() in my netrc credential > helper. But I would formalize it a little more about the token names > and output, Can you elaborate on this? The idea of the Perl code was to mimick a call to the C API, keeping essentially the same names. > and I wouldn't necessarily die() on error. Sure, die()ing in a library is bad. > Maybe this can be merged with the netrc credential helper's > read_credential_data_from_stdin() and print_credential_data()? I don't know about the netrc credential helper, but I guess that's another layer. The git-remote-mediawiki code is the code to call the credential C API, that in turn may (or may not) call a credential helper. > Let me know if you'd like me to libify this... I'm happy to leave it to > Matthieu or Michal, or anyone else interested. I'd happily let you do the job, but I can help if needed. One thing to be careful about: git-remote-mediawiki is currently a standalone script, so it can be installed with a plain "cp git-remote-mediawiki $somewhere/". One consequence of libification is that it adds a dependency on the library (e.g. Git.pm). We should be carefull to keep it easy for the user to install it (e.g. some kind of "make install", or update the doc). -- Matthieu Moy http://www-verimag.imag.fr/~moy/ -- 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: [RFC/PATCH 3/4] grep: allow to use textconv filters
Michael J Gruber writes: > Introduce an option "--textconv" which makes git grep use any configured > textconv filters for grepping and output purposes. It is off by default. > > Signed-off-by: Michael J Gruber > --- > builtin/grep.c | 2 ++ > grep.c | 100 > + > grep.h | 1 + Don't forget to update Documentation/git-grep.txt too. -- Matthieu Moy http://www-verimag.imag.fr/~moy/ -- 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: Bug in "git log --graph -p -m" (version 1.7.7.6)
On Wed, Feb 06, 2013 at 10:03:00AM -0500, Dale R. Worley wrote: > > From: Matthieu Moy > > > > In any case, I can't reproduce with 1.8.1.2.526.gf51a757: I don't get > > undless output. On the other hand, I get a slightly misformatted output: > > > > * commit a393ed598e9fb11436f85bd58f1a38c82f2cadb7 (from > > 2c1e6a36f4b712e914fac994463da7d0fdb2bc6d) > > |\ Merge: 2c1e6a3 33e70e7 > > | | Author: Matthieu Moy > > | | Date: Tue Feb 5 22:05:33 2013 +0100 > > | | > > | | Commit S > > | | > > | | diff --git a/file b/file > > | | index 6bb4d3e..afd2e75 100644 > > | | --- a/file > > | | +++ b/file > > | | @@ -1,4 +1,5 @@ > > | | 1 > > | | 1a > > | | 2 > > | | +2a > > | | 3 > > | | > > commit a393ed598e9fb11436f85bd58f1a38c82f2cadb7 (from > > 33e70e70c0173d634826b998bdc304f93c0966b8) > > | | Merge: 2c1e6a3 33e70e7 > > | | Author: Matthieu Moy > > | | Date: Tue Feb 5 22:05:33 2013 +0100 > > > > The second "commit" line (diff with second parent) doesn't have the > > "| |" prefix, I don't think this is intentional. > > The second "commit" line should start with "| * ": No. That would indicate a commit on the branch that is the second parent of the first commit. But this is the same commit as the one above, just with a diff against its second parent instead of its first parent. I would argue that the line should start with "| | ", since it really is just a continuation of the same commit. | | | | commit a393ed598e9fb11436f85bd58f1a38c82f2cadb7 (from 33e70e70c0173d634826b998bdc304f93c0966b8) | | Merge: 2c1e6a3 33e70e7 | | Author: Matthieu Moy | | Date: Tue Feb 5 22:05:33 2013 +0100 John -- 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 v2 2/2] i18n: mark OPTION_NUMBER (-NUM) for translation
Duy Nguyen writes: > How about utf8_fwprintf? wprintf() deals with wide characters and > returns the number of wide characters, I think the name fits. And we > could just drop utf8_ and use the existing name, because we don't use > wchar_t* anyway. Please, no. That line of reasoning shows a horrible design taste (or lack of taste). "We don't use X right now" (or "We will promise never to use X", for that matter) is never a good reason to abuse a name that normal people would closely associate with X to something that is completely different. That leads to more confusion, not less. I guess utf8_fprintf() is not so bad after all. fprintf() without the utf8_ prefix is perfectly capable of showing a string encoded in UTF-8, and anybody can correctly guess that the magic utf8_ prefix would introduce (i.e. the difference between utf8_fprintf and fprintf) can only be about the return value. It can be reasonably expected that everybody would then know that the display column count can be the only sane return value that is different from what fprintf() would return. -- 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] Verify Content-Type from smart HTTP servers
Jeff King writes: > Is it worth having a strbuf_set* family of functions to match the > strbuf_add*? We seem to have these sorts of errors with strbuf from time > to time, and I wonder if that would make it easier (and more readable) > to do the right thing. Possibly. The callsite below may be a poor example, though; you would need the _reset() even if you change the _addstr() we can see in the context to _setstr() to make sure later strbuf_*(type) will start from a clean slate when !t anyway, no? > > http.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/http.c b/http.c > index d868d8b..d9d1aad 100644 > --- a/http.c > +++ b/http.c > @@ -841,6 +841,7 @@ static int http_request(const char *url, struct strbuf > *type, > > if (type) { > char *t; > + strbuf_reset(type); > curl_easy_getinfo(slot->curl, CURLINFO_CONTENT_TYPE, &t); > if (t) > strbuf_addstr(type, t); -- 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 v3 3/8] upload/receive-pack: allow hiding ref hierarchies
Jeff King writes: > On Tue, Feb 05, 2013 at 07:45:01AM -0800, Junio C Hamano wrote: > >> > In the earlier review, I mentioned making this per-service, but I see >> > that is not the case here. Do you have an argument against doing so? >> >> Perhaps then I misunderstood your intention. By reminding me of the >> receive-pack side, I thought you were hinting to unify these two >> into one, which I did. There is no argument against it. > > What I meant was that there should be transfer.hiderefs, and an > individual {receive,uploadpack}.hiderefs, similar to the way we have > transfer.unpacklimit. Yes, as I said, I misunderstood your intention. -- 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
How to diff 2 file revisions with gitk
Hi there: I asked a few days ago whether I could easily diff 2 file revisions with the mouse in gitk, but I got no reply yet, see here: How to diff two file revisions with the mouse (with gitk) https://groups.google.com/forum/#!topic/git-users/9znsQsTB0dE I am hoping that it was the wrong mailing list, and this one the right one. 8-) Here is the full question text again: 8<8<8<8< I would like to start gitk, select with the mouse 2 revisions of some file and then compare them, hopefully with an external diff tool, very much like I am used to with WinCVS. The closest I got is to start gitk with a filename as an argument, in order to restrict the log to that one file. Then I right-click on a commit (a file revision) and choose "Mark this commit". However, if I right-click on another commit and choose "Compare with marked commit", I get a full commit diff with all files, and not just the file I specified on the command-line arguments. Selecting a filename in the "Tree" view and choosing "Highlight this only", as I found on the Internet, does not seem to help. I have git 1.7.9 (on Cygwin). Can someone help? By the way, it would be nice if gitk could launch the external diff tool from the "Compare with marked commit" option too. 8<8<8<8< Thanks in advance, rdiez -- 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] git-send-email: add ~/.authinfo parsing
On Wed, 06 Feb 2013 16:10:12 +0100 Matthieu Moy wrote: MM> Ted Zlatanov writes: MM> [...] so the way to go for send-email is probably to libify the MM> credential support in git-remote-mediawiki, and to use it in send-email. >> >> I looked and that's indeed very useful. If it's put in a library, I'd >> use credential_read() and credential_write() in my netrc credential >> helper. But I would formalize it a little more about the token names >> and output, MM> Can you elaborate on this? The idea of the Perl code was to mimick a MM> call to the C API, keeping essentially the same names. None of these are a big deal, and Michal said he's working on libifying this anyhow: - making 'fill' a special operation is weird - anchor the key regex to beginning of line (not strictly necessary) - sort the output tokens (after 'url' is extracted) so the output is consistent and testable >> and I wouldn't necessarily die() on error. MM> Sure, die()ing in a library is bad. >> Maybe this can be merged with the netrc credential helper's >> read_credential_data_from_stdin() and print_credential_data()? MM> I don't know about the netrc credential helper, but I guess that's MM> another layer. The git-remote-mediawiki code is the code to call the MM> credential C API, that in turn may (or may not) call a credential MM> helper. Yup. But what you call "read" and "write" are, to the credential helper, "write" and "read" but it's the same protocol :) So maybe the names should be changed to reflect that, e.g. "query" and "response." MM> One thing to be careful about: git-remote-mediawiki is currently a MM> standalone script, so it can be installed with a plain "cp MM> git-remote-mediawiki $somewhere/". One consequence of libification MM> is that it adds a dependency on the library (e.g. Git.pm). We should MM> be carefull to keep it easy for the user to install it (e.g. some MM> kind of "make install", or update the doc). I don't know--it's up to the `git-remote-mediawiki' maintainers... But I think anywhere you have Git, you also have Git.pm, right? Maybe? But then you also have to look at whether Git.pm has the functionality you need... so I better go quiet :) Ted -- 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
[Request] Git export with hardlinks
Hi, I'd like to script a git export command that can be given a list of already exported worktrees and the tree SHA1s these worktrees correspond too. The git export command should then for every file it wants to export lookup in the existing worktrees whether an identical file is already present and in that case hardlink to the new export location instead of writing the same file again. Use Case: A git based web deployment system that exports git trees to be served by a web server. Every new deployment is written to a new folder. After the export the web server should start serving new requests from the new folder. It might be possible that this is premature optimization. But I'd like to learn more Python and dulwich by hacking this. Do you have any additional thoughts or use cases about this? Regards, Thomas Koch, http://www.koch.ro -- 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: CodingGuidelines Perl amendment
Ted Zlatanov writes: > - As in C (see above), we avoid using braces unnecessarily (but Perl >forces braces around if/unless/else/foreach blocks, so this is not >always possible). Is it ever (as opposed to "not always") possible to omit braces? It sounds as if we encourage the use of statement modifiers, which certainly is not what I want to see. You probably would want to mention that opening braces for "if/else/elsif" do not sit on their own line, and closing braces for them will be followed the next "else/elseif" on the same line instead, but that is part of "most of the C guidelines above apply" so it may be redundant. > - Don't abuse statement modifiers (unless $youmust). It does not make a useful guidance to leave $youmust part unspecified. Incidentally, your sentence is a good example of where use of statement modifiers is appropriate: $youmust is rarely true. In general: ... do something ... do_this() unless (condition); ... do something else ... is easier to follow the flow of the logic than ... do something ... unless (condition) { do_this(); } ... do something else ... *only* when condition is extremely rare, iow, when do_this() is expected to be almost always called. -- 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] git-send-email: add ~/.authinfo parsing
Ted Zlatanov writes: > None of these are a big deal, and Michal said he's working on libifying > this anyhow: > > - making 'fill' a special operation is weird Well, 'fill' is the only operation that mutates the credential structure (i.e. the only one for which "git credential" emits an output to be parsed), so you don't have much choice. > - anchor the key regex to beginning of line (not strictly necessary) Right. The greedyness of * ensures correction, but I like explicit anchors ^...$ too. > - sort the output tokens (after 'url' is extracted) so the output is > consistent and testable Why not, if you want to use the output of credential_write in tests. But credential_write is essentially used to talk to "git credential", so the important information is the content of the hash before credential_write and after credential_read. They are unordered, but consistent and testable. >>> Maybe this can be merged with the netrc credential helper's >>> read_credential_data_from_stdin() and print_credential_data()? > > MM> I don't know about the netrc credential helper, but I guess that's > MM> another layer. The git-remote-mediawiki code is the code to call the > MM> credential C API, that in turn may (or may not) call a credential > MM> helper. > > Yup. But what you call "read" and "write" are, to the credential > helper, "write" and "read" but it's the same protocol :) So maybe the > names should be changed to reflect that, e.g. "query" and "response." I don't think that would be a better naming. Maybe "serialize" and "parse" would be better, but "query" would sound like it establishes the connection and possibly reads the response to me. > MM> One thing to be careful about: git-remote-mediawiki is currently a > MM> standalone script, so it can be installed with a plain "cp > MM> git-remote-mediawiki $somewhere/". One consequence of libification > MM> is that it adds a dependency on the library (e.g. Git.pm). We should > MM> be carefull to keep it easy for the user to install it (e.g. some > MM> kind of "make install", or update the doc). > > I don't know--it's up to the `git-remote-mediawiki' maintainers... That is, me ;-). > But I think anywhere you have Git, you also have Git.pm, right? Yes, but you have to find out where it is installed. Git's Makefile hardcodes the path to Git.pm at build time, inserting one line in the perl script: use lib (split(/:/, $ENV{GITPERLLIB} || "$INSTLIBDIR")); The same needs to be done for git-remote-mediawiki. As much as possible, I'd rather avoid copy-pasting from Git's Makefile, so this means extracting the perl part of Git's Makefile and make it available in contrib/. I'll try a patch in this direction. > Maybe? But then you also have to look at whether Git.pm has the > functionality you need... If git-remote-mediawiki is installed from Git's source, I think it's OK to assume that Git.pm will be up to date, but that would be even better if we can issue a clean error message when the functions to be called do not exist. -- Matthieu Moy http://www-verimag.imag.fr/~moy/ -- 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: [RFC/PATCH 2/4] cat-file: do not die on --textconv without textconv filters
Michael J Gruber writes: > When a command is supposed to use textconv filters (by default or with > "--textconv") and none are configured then the blob is output without > conversion; the only exception to this rule is "cat-file --textconv". I am of two minds. Because cat-file is mostly a low-level plumbing, I do not necessarily think it is a bad behaviour for it to error out when it was asked to apply textconv where there is no filter or when the filter fails to produce an output. On the other hand, it certainly makes it more convenient for callers that do not care too deeply, taking textconv as a mere hint just like Porcelains do. > > Make it behave like the rest of textconv aware commands. > > Signed-off-by: Michael J Gruber > --- > builtin/cat-file.c | 9 + > t/t8007-cat-file-textconv.sh | 20 +--- > 2 files changed, 10 insertions(+), 19 deletions(-) > > diff --git a/builtin/cat-file.c b/builtin/cat-file.c > index 00528dd..6912dc2 100644 > --- a/builtin/cat-file.c > +++ b/builtin/cat-file.c > @@ -146,10 +146,11 @@ static int cat_one_file(int opt, const char *exp_type, > const char *obj_name) > die("git cat-file --textconv %s: must be > ", > obj_name); > > - if (!textconv_object(obj_context.path, obj_context.mode, sha1, > 1, &buf, &size)) > - die("git cat-file --textconv: unable to run textconv on > %s", > - obj_name); > - break; > + if (textconv_object(obj_context.path, obj_context.mode, sha1, > 1, &buf, &size)) > + break; > + > + /* otherwise expect a blob */ > + exp_type = "blob"; Please use the constant string blob_type that is available for all callers including this one. But stepping back a bit. What happens when I say "cat-file -c HEAD:Documentation", and what should happen when I do so? I think what we want to see in the ideal world might be: * If we have a textconv for tree objects at that path to format it specially, textconv_object() may be allowed to textualize it (even though it is not a blob, and textconv so far has always been about blobs; it needs to be considered carefully if it makes sense to allow such a usage) and show it; * If we don't, we act as if -c were -p; in other words, we treat the built-in "human output" implemented by "cat-file -p" as if that is a textconv. In other words, you may want to fall-thru to case 'p', not case 0 with forced "blob" type. -- 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: [RFC/PATCH 1/4] show: obey --textconv for blobs
Michael J Gruber writes: > Currently, "diff" and "cat-file" for blobs obey "--textconv" options > (with the former defaulting to "--textconv" and the latter to > "--no-textconv") whereas "show" does not obey this option, even though > it takes diff options. > > Make "show" on blobs behave like "diff", i.e. obey "--textconv" by > default and "--no-textconv" when given. What does "log -p" do currently, and what should it do? Does/should it also use --textconv? The --textconv is a natural extension of what --ext-diff provides us, so I think it should trigger the same way as how --ext-diff triggers. We apply "--ext-diff" for "diff" by default but not for "log -p" and "show"; I suspect this may have been for a good reason but I do not recall the discussion that led to the current behaviour offhand. > Signed-off-by: Michael J Gruber > --- > builtin/log.c | 24 +--- > 1 file changed, 21 insertions(+), 3 deletions(-) > > diff --git a/builtin/log.c b/builtin/log.c > index 8f0b2e8..f83870d 100644 > --- a/builtin/log.c > +++ b/builtin/log.c > @@ -402,10 +402,28 @@ static void show_tagger(char *buf, int len, struct > rev_info *rev) > strbuf_release(&out); > } > > -static int show_blob_object(const unsigned char *sha1, struct rev_info *rev) > +static int show_blob_object(const unsigned char *sha1, struct rev_info *rev, > const char *obj_name) > { > + unsigned char sha1c[20]; > + struct object_context obj_context; > + char *buf; > + unsigned long size; > + > fflush(stdout); > - return stream_blob_to_fd(1, sha1, NULL, 0); > + if (!DIFF_OPT_TST(&rev->diffopt, ALLOW_TEXTCONV)) > + return stream_blob_to_fd(1, sha1, NULL, 0); > + > + if (get_sha1_with_context(obj_name, 0, sha1c, &obj_context)) > + die("Not a valid object name %s", obj_name); > + if (!obj_context.path[0] || > + !textconv_object(obj_context.path, obj_context.mode, sha1c, 1, > &buf, &size)) > + return stream_blob_to_fd(1, sha1, NULL, 0); > + > + if (!buf) > + die("git show %s: bad file", obj_name); > + > + write_or_die(1, buf, size); > + return 0; > } > > static int show_tag_object(const unsigned char *sha1, struct rev_info *rev) > @@ -491,7 +509,7 @@ int cmd_show(int argc, const char **argv, const char > *prefix) > const char *name = objects[i].name; > switch (o->type) { > case OBJ_BLOB: > - ret = show_blob_object(o->sha1, NULL); > + ret = show_blob_object(o->sha1, &rev, name); > break; > case OBJ_TAG: { > struct tag *t = (struct tag *)o; -- 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: [RFC/PATCH 0/4] textconv for show and grep
The parts for "grep" in the series makes tons of sense to me. I am not yet convinced about the other two, though. Thanks. -- 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] git-send-email: add ~/.authinfo parsing
On Wed, 06 Feb 2013 17:41:01 +0100 Matthieu Moy wrote: MM> Ted Zlatanov writes: >> - sort the output tokens (after 'url' is extracted) so the output is >> consistent and testable MM> Why not, if you want to use the output of credential_write in tests. But MM> credential_write is essentially used to talk to "git credential", so the MM> important information is the content of the hash before credential_write MM> and after credential_read. They are unordered, but consistent and MM> testable. I like testing output (especially when it's part of an API), so we should make the externally observable output consistent and testable. The change is tiny, just sort the keys instead of calling each(), so I hope it makes it in the final version. >> Yup. But what you call "read" and "write" are, to the credential >> helper, "write" and "read" but it's the same protocol :) So maybe the >> names should be changed to reflect that, e.g. "query" and "response." MM> I don't think that would be a better naming. Maybe "serialize" and MM> "parse" would be better, but "query" would sound like it establishes the MM> connection and possibly reads the response to me. I'm OK with anything unambiguous. Thanks! Ted -- 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: CodingGuidelines Perl amendment
On 6 February 2013 17:29, Junio C Hamano wrote: > Ted Zlatanov writes: > >> - As in C (see above), we avoid using braces unnecessarily (but Perl >>forces braces around if/unless/else/foreach blocks, so this is not >>always possible). > > Is it ever (as opposed to "not always") possible to omit braces? Only in a statement modifier. > It sounds as if we encourage the use of statement modifiers, which > certainly is not what I want to see. As you mention below statement modifiers have their place. For instance next if $whatever; Is considered preferable to if ($whatever) { next; } Similarly open my $fh, ">", $filename or die "Failed to open '$filename': $!"; Is considered preferable by most Perl programmers to: my $fh; if ( not open $fh, ">", $filename ) { die "Failed to open '$filename': $!"; } > You probably would want to mention that opening braces for > "if/else/elsif" do not sit on their own line, > and closing braces for > them will be followed the next "else/elseif" on the same line > instead, but that is part of "most of the C guidelines above apply" > so it may be redundant. > >> - Don't abuse statement modifiers (unless $youmust). > > It does not make a useful guidance to leave $youmust part > unspecified. > > Incidentally, your sentence is a good example of where use of > statement modifiers is appropriate: $youmust is rarely true. "unless" often leads to maintenance errors as the expression gets more complicated over time, more branches need to be added to the statement, etc. Basically people are bad at doing De Morgans law in their head. > In general: > > ... do something ... > do_this() unless (condition); > ... do something else ... > > is easier to follow the flow of the logic than > > ... do something ... > unless (condition) { > do_this(); > } > ... do something else ... > > *only* when condition is extremely rare, iow, when do_this() is > expected to be almost always called. if (not $condition) { do_this(); } Is much less error prone in terms of maintenance than unless ($condition) { do_this(); } Similarly do_this() if not $condition; leads to less maintenance errors than do_this() unless $condition; So if you objective is maintainability I would just ban "unless" outright. Cheers, Yves -- perl -Mre=debug -e "/just|another|perl|hacker/" -- 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] Update CodingGuidelines for Perl 5
Update the coding guidelines for Perl 5. Signed-off-by: Ted Zlatanov --- Documentation/CodingGuidelines | 44 1 files changed, 44 insertions(+), 0 deletions(-) diff --git a/Documentation/CodingGuidelines b/Documentation/CodingGuidelines index 1d7de5f..951d74c 100644 --- a/Documentation/CodingGuidelines +++ b/Documentation/CodingGuidelines @@ -179,6 +179,50 @@ For C programs: - Use Git's gettext wrappers to make the user interface translatable. See "Marking strings for translation" in po/README. +For Perl 5 programs: + + - Most of the C guidelines above apply. + + - We try to support Perl 5.8 and later ("use Perl 5.008"). + + - use strict and use warnings are strongly preferred. + + - As in C (see above), we avoid using braces unnecessarily (but Perl forces + braces around if/unless/else/foreach blocks, so this is not always possible). + At least make sure braces do not sit on their own line, like with C. + + - Don't abuse statement modifiers--they are discouraged. But in general: + + ... do something ... + do_this() unless (condition); +... do something else ... + + should be used instead of + + ... do something ... + unless (condition) { + do_this(); + } +... do something else ... + + *only* when when the condition is so rare that do_this() will be called + almost always. + + - We try to avoid assignments inside if(). + + - Learn and use Git.pm if you need that functionality. + + - For Emacs, it's useful to put the following in + GIT_CHECKOUT/.dir-locals.el, assuming you use cperl-mode: + +;; note the first part is useful for C editing, too +((nil . ((indent-tabs-mode . t) + (tab-width . 8) + (fill-column . 80))) + (cperl-mode . ((cperl-indent-level . 8) +(cperl-extra-newline-before-brace . nil) +(cperl-merge-trailing-else . t + Writing Documentation: Every user-visible change should be reflected in the documentation. -- 1.7.9.rc2 -- 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: CodingGuidelines Perl amendment
On Wed, 06 Feb 2013 08:29:30 -0800 Junio C Hamano wrote: JCH> Is it ever (as opposed to "not always") possible to omit braces? Oh yes! Not that I recommend it, and I'm not even going to touch on Perl Golf :) JCH> It sounds as if we encourage the use of statement modifiers, which JCH> certainly is not what I want to see. Yup. I think I captured that in the patch, but please feel free to revise it after applying or throw it back to me. JCH> You probably would want to mention that opening braces for JCH> "if/else/elsif" do not sit on their own line, and closing braces for JCH> them will be followed the next "else/elseif" on the same line JCH> instead, but that is part of "most of the C guidelines above apply" JCH> so it may be redundant. OK; done. >> - Don't abuse statement modifiers (unless $youmust). JCH> It does not make a useful guidance to leave $youmust part JCH> unspecified. JCH> Incidentally, your sentence is a good example of where use of JCH> statement modifiers is appropriate: $youmust is rarely true. I was trying to be funny, honestly. But OK; reworded. Ted -- 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: CodingGuidelines Perl amendment
On Wed, 6 Feb 2013 18:45:56 +0100 demerphq wrote: d> So if you objective is maintainability I would just ban "unless" outright. Please consider me opposed to such a ban. Ted -- 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: How to diff 2 file revisions with gitk
Am 06.02.2013 16:57, schrieb R. Diez: > I would like to start gitk, select with the mouse 2 > revisions of some file and then compare them, hopefully with an external > diff tool, very much like I am used to with WinCVS. > > The closest I > got is to start gitk with a filename as an argument, in order to > restrict the log to that one file. Then I right-click on a commit (a > file revision) and choose "Mark this commit". However, if I right-click > on another commit and choose "Compare with marked commit", I get a full > commit diff with all files, and not just the file I specified on the > command-line arguments. Edit->Preferences, tick 'Limit diff to listed paths'. -- Hannes -- 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 2/4] perl.mak: introduce $(GIT_ROOT_DIR) to allow inclusion from other directories
perl.mak uses relative path, which is OK when called from the toplevel, but won't be anymore if one includes it from elsewhere. It is now possible to include the file using: GIT_ROOT_DIR= include $(GIT_ROOT_DIR)/perl.mak Signed-off-by: Matthieu Moy --- perl.mak | 11 +++ 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/perl.mak b/perl.mak index 8bbeef3..a2b8717 100644 --- a/perl.mak +++ b/perl.mak @@ -1,5 +1,9 @@ # Rules to build Git commands written in perl +ifndef GIT_ROOT_DIR + GIT_ROOT_DIR = . +endif + ifndef PERL_PATH PERL_PATH = /usr/bin/perl endif @@ -11,12 +15,11 @@ NO_PERL = NoThanks endif ifndef NO_PERL -$(patsubst %.perl,%,$(SCRIPT_PERL)): perl/perl.mak - +$(patsubst %.perl,%,$(SCRIPT_PERL)): $(GIT_ROOT_DIR)/perl/perl.mak -$(patsubst %.perl,%,$(SCRIPT_PERL)): % : %.perl GIT-VERSION-FILE +$(patsubst %.perl,%,$(SCRIPT_PERL)): % : %.perl $(GIT_ROOT_DIR)/GIT-VERSION-FILE $(QUIET_GEN)$(RM) $@ $@+ && \ - INSTLIBDIR=`MAKEFLAGS= $(MAKE) -C perl -s --no-print-directory instlibdir` && \ + INSTLIBDIR=`MAKEFLAGS= $(MAKE) -C $(GIT_ROOT_DIR)/perl -s --no-print-directory instlibdir` && \ sed -e '1{' \ -e 's|#!.*perl|#!$(PERL_PATH_SQ)|' \ -e 'h' \ -- 1.8.1.2.526.gf51a757 -- 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 1/4] Makefile: extract perl-related rules to make them available from other dirs
The final goal is to make it easy to write Git commands in perl in the contrib/ directory. It is currently possible to do so, but without the benefits of Git's Makefile: adapt first line with $(PERL_PATH), hardcode the path to Git.pm, ... We make the perl-related part of the Makefile available from directories other than the toplevel so that: * Developers can include it, to avoid code duplication * Users can get a consistent behavior of "make install" Signed-off-by: Matthieu Moy --- Makefile | 46 +- perl.mak | 49 + 2 files changed, 50 insertions(+), 45 deletions(-) create mode 100644 perl.mak diff --git a/Makefile b/Makefile index 731b6a8..f39d4a9 100644 --- a/Makefile +++ b/Makefile @@ -573,14 +573,10 @@ BINDIR_PROGRAMS_NO_X += git-cvsserver ifndef SHELL_PATH SHELL_PATH = /bin/sh endif -ifndef PERL_PATH - PERL_PATH = /usr/bin/perl -endif ifndef PYTHON_PATH PYTHON_PATH = /usr/bin/python endif -export PERL_PATH export PYTHON_PATH LIB_FILE = libgit.a @@ -1441,10 +1437,6 @@ ifeq ($(TCLTK_PATH),) NO_TCLTK = NoThanks endif -ifeq ($(PERL_PATH),) -NO_PERL = NoThanks -endif - ifeq ($(PYTHON_PATH),) NO_PYTHON = NoThanks endif @@ -1522,7 +1514,6 @@ prefix_SQ = $(subst ','\'',$(prefix)) gitwebdir_SQ = $(subst ','\'',$(gitwebdir)) SHELL_PATH_SQ = $(subst ','\'',$(SHELL_PATH)) -PERL_PATH_SQ = $(subst ','\'',$(PERL_PATH)) PYTHON_PATH_SQ = $(subst ','\'',$(PYTHON_PATH)) TCLTK_PATH_SQ = $(subst ','\'',$(TCLTK_PATH)) DIFF_SQ = $(subst ','\'',$(DIFF)) @@ -1715,9 +1706,6 @@ $(SCRIPT_LIB) : % : %.sh GIT-SCRIPT-DEFINES $(QUIET_GEN)$(cmd_munge_script) && \ mv $@+ $@ -ifndef NO_PERL -$(patsubst %.perl,%,$(SCRIPT_PERL)): perl/perl.mak - perl/perl.mak: perl/PM.stamp perl/PM.stamp: FORCE @@ -1728,39 +1716,7 @@ perl/PM.stamp: FORCE perl/perl.mak: GIT-CFLAGS GIT-PREFIX perl/Makefile perl/Makefile.PL $(QUIET_SUBDIR0)perl $(QUIET_SUBDIR1) PERL_PATH='$(PERL_PATH_SQ)' prefix='$(prefix_SQ)' $(@F) -$(patsubst %.perl,%,$(SCRIPT_PERL)): % : %.perl GIT-VERSION-FILE - $(QUIET_GEN)$(RM) $@ $@+ && \ - INSTLIBDIR=`MAKEFLAGS= $(MAKE) -C perl -s --no-print-directory instlibdir` && \ - sed -e '1{' \ - -e 's|#!.*perl|#!$(PERL_PATH_SQ)|' \ - -e 'h' \ - -e 's=.*=use lib (split(/$(pathsep)/, $$ENV{GITPERLLIB} || "'"$$INSTLIBDIR"'"));=' \ - -e 'H' \ - -e 'x' \ - -e '}' \ - -e 's/@@GIT_VERSION@@/$(GIT_VERSION)/g' \ - $@.perl >$@+ && \ - chmod +x $@+ && \ - mv $@+ $@ - - -.PHONY: gitweb -gitweb: - $(QUIET_SUBDIR0)gitweb $(QUIET_SUBDIR1) all - -git-instaweb: git-instaweb.sh gitweb GIT-SCRIPT-DEFINES - $(QUIET_GEN)$(cmd_munge_script) && \ - chmod +x $@+ && \ - mv $@+ $@ -else # NO_PERL -$(patsubst %.perl,%,$(SCRIPT_PERL)) git-instaweb: % : unimplemented.sh - $(QUIET_GEN)$(RM) $@ $@+ && \ - sed -e '1s|#!.*/sh|#!$(SHELL_PATH_SQ)|' \ - -e 's|@@REASON@@|NO_PERL=$(NO_PERL)|g' \ - unimplemented.sh >$@+ && \ - chmod +x $@+ && \ - mv $@+ $@ -endif # NO_PERL +include perl.mak ifndef NO_PYTHON $(patsubst %.py,%,$(SCRIPT_PYTHON)): GIT-CFLAGS GIT-PREFIX GIT-PYTHON-VARS diff --git a/perl.mak b/perl.mak new file mode 100644 index 000..8bbeef3 --- /dev/null +++ b/perl.mak @@ -0,0 +1,49 @@ +# Rules to build Git commands written in perl + +ifndef PERL_PATH + PERL_PATH = /usr/bin/perl +endif +export PERL_PATH +PERL_PATH_SQ = $(subst ','\'',$(PERL_PATH)) + +ifeq ($(PERL_PATH),) +NO_PERL = NoThanks +endif + +ifndef NO_PERL +$(patsubst %.perl,%,$(SCRIPT_PERL)): perl/perl.mak + + +$(patsubst %.perl,%,$(SCRIPT_PERL)): % : %.perl GIT-VERSION-FILE + $(QUIET_GEN)$(RM) $@ $@+ && \ + INSTLIBDIR=`MAKEFLAGS= $(MAKE) -C perl -s --no-print-directory instlibdir` && \ + sed -e '1{' \ + -e 's|#!.*perl|#!$(PERL_PATH_SQ)|' \ + -e 'h' \ + -e 's=.*=use lib (split(/$(pathsep)/, $$ENV{GITPERLLIB} || "'"$$INSTLIBDIR"'"));=' \ + -e 'H' \ + -e 'x' \ + -e '}' \ + -e 's/@@GIT_VERSION@@/$(GIT_VERSION)/g' \ + $@.perl >$@+ && \ + chmod +x $@+ && \ + mv $@+ $@ + + +.PHONY: gitweb +gitweb: + $(QUIET_SUBDIR0)gitweb $(QUIET_SUBDIR1) all + +git-instaweb: git-instaweb.sh gitweb GIT-SCRIPT-DEFINES + $(QUIET_GEN)$(cmd_munge_script) && \ + chmod +x $@+ && \ + mv $@+ $@ +else # NO_PERL +$(patsubst %.perl,%,$(SCRIPT_PERL)) git-instaweb: % : unimplemented.sh + $(QUIET_GEN)$(RM) $@ $@+ && \ + sed -e '1s|#!.*/sh|#!$(SHELL_PATH_SQ)|' \ + -e 's|@@REASON@@|NO_PERL=$(NO_PERL)|g' \ + unimplemented.sh >$@+ && \ + chmod +x $@+ && \ + mv $@+ $@ +endif # NO_PERL -- 1.8.1.2.526.gf51a757 -- To unsubscribe
[PATCH 3/4] Makefile: factor common configuration in git-default-config.mak
Similarly to the extraction of perl-related code in perl.mak, we extract general default configuration from the Makefile to make it available from directories other than the toplevel. This is required to make perl.mak usable because it requires $(pathsep) to be set. Signed-off-by: Matthieu Moy --- Makefile | 62 +- default-config.mak | 61 + 2 files changed, 62 insertions(+), 61 deletions(-) create mode 100644 default-config.mak diff --git a/Makefile b/Makefile index f39d4a9..9649a41 100644 --- a/Makefile +++ b/Makefile @@ -346,67 +346,7 @@ GIT-VERSION-FILE: FORCE @$(SHELL_PATH) ./GIT-VERSION-GEN -include GIT-VERSION-FILE -# CFLAGS and LDFLAGS are for the users to override from the command line. - -CFLAGS = -g -O2 -Wall -LDFLAGS = -ALL_CFLAGS = $(CPPFLAGS) $(CFLAGS) -ALL_LDFLAGS = $(LDFLAGS) -STRIP ?= strip - -# Among the variables below, these: -# gitexecdir -# template_dir -# mandir -# infodir -# htmldir -# sysconfdir -# can be specified as a relative path some/where/else; -# this is interpreted as relative to $(prefix) and "git" at -# runtime figures out where they are based on the path to the executable. -# This can help installing the suite in a relocatable way. - -prefix = $(HOME) -bindir_relative = bin -bindir = $(prefix)/$(bindir_relative) -mandir = share/man -infodir = share/info -gitexecdir = libexec/git-core -mergetoolsdir = $(gitexecdir)/mergetools -sharedir = $(prefix)/share -gitwebdir = $(sharedir)/gitweb -localedir = $(sharedir)/locale -template_dir = share/git-core/templates -htmldir = share/doc/git-doc -ETC_GITCONFIG = $(sysconfdir)/gitconfig -ETC_GITATTRIBUTES = $(sysconfdir)/gitattributes -lib = lib -# DESTDIR = -pathsep = : - -export prefix bindir sharedir sysconfdir gitwebdir localedir - -CC = cc -AR = ar -RM = rm -f -DIFF = diff -TAR = tar -FIND = find -INSTALL = install -RPMBUILD = rpmbuild -TCL_PATH = tclsh -TCLTK_PATH = wish -XGETTEXT = xgettext -MSGFMT = msgfmt -PTHREAD_LIBS = -lpthread -PTHREAD_CFLAGS = -GCOV = gcov - -export TCL_PATH TCLTK_PATH - -SPARSE_FLAGS = - - +include default-config.mak ### --- END CONFIGURATION SECTION --- diff --git a/default-config.mak b/default-config.mak new file mode 100644 index 000..b2aab3d --- /dev/null +++ b/default-config.mak @@ -0,0 +1,61 @@ +# CFLAGS and LDFLAGS are for the users to override from the command line. + +CFLAGS = -g -O2 -Wall +LDFLAGS = +ALL_CFLAGS = $(CPPFLAGS) $(CFLAGS) +ALL_LDFLAGS = $(LDFLAGS) +STRIP ?= strip + +# Among the variables below, these: +# gitexecdir +# template_dir +# mandir +# infodir +# htmldir +# sysconfdir +# can be specified as a relative path some/where/else; +# this is interpreted as relative to $(prefix) and "git" at +# runtime figures out where they are based on the path to the executable. +# This can help installing the suite in a relocatable way. + +prefix = $(HOME) +bindir_relative = bin +bindir = $(prefix)/$(bindir_relative) +mandir = share/man +infodir = share/info +gitexecdir = libexec/git-core +mergetoolsdir = $(gitexecdir)/mergetools +sharedir = $(prefix)/share +gitwebdir = $(sharedir)/gitweb +localedir = $(sharedir)/locale +template_dir = share/git-core/templates +htmldir = share/doc/git-doc +ETC_GITCONFIG = $(sysconfdir)/gitconfig +ETC_GITATTRIBUTES = $(sysconfdir)/gitattributes +lib = lib +# DESTDIR = +pathsep = : + +export prefix bindir sharedir sysconfdir gitwebdir localedir + +CC = cc +AR = ar +RM = rm -f +DIFF = diff +TAR = tar +FIND = find +INSTALL = install +RPMBUILD = rpmbuild +TCL_PATH = tclsh +TCLTK_PATH = wish +XGETTEXT = xgettext +MSGFMT = msgfmt +PTHREAD_LIBS = -lpthread +PTHREAD_CFLAGS = +GCOV = gcov + +export TCL_PATH TCLTK_PATH + +SPARSE_FLAGS = + + -- 1.8.1.2.526.gf51a757 -- 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 0/4] Allow contrib/ to use Git's Makefile for perl code
The very final goal is to be able to move painlessly (credential) code from git-remote-mediawiki to Git.pm, but then it's nice for the user to be able to say just "cd contrib/mw-to-git && make install" and let the Makefile set perl's library path just like other Git commands written in perl. This series does this while trying to minimize code duplication, and to make it easy for future other tools in contrib to do the same. Matthieu Moy (4): Makefile: extract perl-related rules to make them available from other dirs perl.mak: introduce $(GIT_ROOT_DIR) to allow inclusion from other directories Makefile: factor common configuration in git-default-config.mak git-remote-mediawiki: use Git's Makefile to build the script Makefile | 108 + contrib/mw-to-git/.gitignore | 1 + contrib/mw-to-git/Makefile | 45 ++--- ...-remote-mediawiki => git-remote-mediawiki.perl} | 0 default-config.mak | 61 perl.mak | 52 ++ 6 files changed, 145 insertions(+), 122 deletions(-) create mode 100644 contrib/mw-to-git/.gitignore rename contrib/mw-to-git/{git-remote-mediawiki => git-remote-mediawiki.perl} (100%) create mode 100644 default-config.mak create mode 100644 perl.mak -- 1.8.1.2.526.gf51a757 -- 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 4/4] git-remote-mediawiki: use Git's Makefile to build the script
The configuration of the install directory is not reused from the toplevel Makefile: we assume Git is already built, hence just call "git --exec-path". This avoids too much surgery in the toplevel Makefile. git-remote-mediawiki.perl can now "use Git;". Signed-off-by: Matthieu Moy --- contrib/mw-to-git/.gitignore | 1 + contrib/mw-to-git/Makefile | 45 ++ ...-remote-mediawiki => git-remote-mediawiki.perl} | 0 3 files changed, 30 insertions(+), 16 deletions(-) create mode 100644 contrib/mw-to-git/.gitignore rename contrib/mw-to-git/{git-remote-mediawiki => git-remote-mediawiki.perl} (100%) diff --git a/contrib/mw-to-git/.gitignore b/contrib/mw-to-git/.gitignore new file mode 100644 index 000..b919655 --- /dev/null +++ b/contrib/mw-to-git/.gitignore @@ -0,0 +1 @@ +git-remote-mediawiki diff --git a/contrib/mw-to-git/Makefile b/contrib/mw-to-git/Makefile index 3ed728b..ed8073b 100644 --- a/contrib/mw-to-git/Makefile +++ b/contrib/mw-to-git/Makefile @@ -8,40 +8,53 @@ # ## Build git-remote-mediawiki --include ../../config.mak.autogen --include ../../config.mak +all: + +GIT_ROOT_DIR=../../ +include $(GIT_ROOT_DIR)/default-config.mak +-include $(GIT_ROOT_DIR)/config.mak.autogen +-include $(GIT_ROOT_DIR)/config.mak +-include $(GIT_ROOT_DIR)/GIT-VERSION-FILE + + +SCRIPT_PERL = git-remote-mediawiki.perl +ALL_PROGRAMS = $(patsubst %.perl,%,$(SCRIPT_PERL)) + +include $(GIT_ROOT_DIR)/perl.mak -ifndef PERL_PATH - PERL_PATH = /usr/bin/perl -endif ifndef gitexecdir gitexecdir = $(shell git --exec-path) endif -PERL_PATH_SQ = $(subst ','\'',$(PERL_PATH)) -gitexecdir_SQ = $(subst ','\'',$(gitexecdir)) -SCRIPT = git-remote-mediawiki +ifneq ($(filter /%,$(firstword $(gitexecdir))),) +gitexec_instdir = $(gitexecdir) +else +gitexec_instdir = $(prefix)/$(gitexecdir) +endif +gitexec_instdir_SQ = $(subst ','\'',$(gitexec_instdir)) .PHONY: install help doc test clean help: @echo 'This is the help target of the Makefile. Current configuration:' - @echo ' gitexecdir = $(gitexecdir_SQ)' + @echo ' gitexec_instdir = $(gitexec_instdir_SQ)' @echo ' PERL_PATH = $(PERL_PATH_SQ)' - @echo 'Run "$(MAKE) install" to install $(SCRIPT) in gitexecdir' + @echo 'Run "$(MAKE) all" to build the script' + @echo 'Run "$(MAKE) install" to install $(ALL_PROGRAMS) in gitexec_instdir' @echo 'Run "$(MAKE) test" to run the testsuite' -install: - sed -e '1s|#!.*/perl|#!$(PERL_PATH_SQ)|' $(SCRIPT) \ - > '$(gitexecdir_SQ)/$(SCRIPT)' - chmod +x '$(gitexecdir)/$(SCRIPT)' +all: $(ALL_PROGRAMS) + +install: $(ALL_PROGRAMS) + $(INSTALL) $(ALL_PROGRAMS) '$(DESTDIR_SQ)$(gitexec_instdir_SQ)' doc: - @echo 'Sorry, "make doc" is not implemented yet for $(SCRIPT)' + @echo 'Sorry, "make doc" is not implemented yet for $(ALL_PROGRAMS)' test: $(MAKE) -C t/ test clean: - $(RM) '$(gitexecdir)/$(SCRIPT)' + $(RM) $(ALL_PROGRAMS) + $(RM) $(patsubst %,$(gitexec_instdir)/%,/$(ALL_PROGRAMS)) $(MAKE) -C t/ clean diff --git a/contrib/mw-to-git/git-remote-mediawiki b/contrib/mw-to-git/git-remote-mediawiki.perl similarity index 100% rename from contrib/mw-to-git/git-remote-mediawiki rename to contrib/mw-to-git/git-remote-mediawiki.perl -- 1.8.1.2.526.gf51a757 -- 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: CodingGuidelines Perl amendment
demerphq writes: > As you mention below statement modifiers have their place. For instance > > next if $whatever; > > Is considered preferable to > > if ($whatever) { > next; > } > > Similarly > > open my $fh, ">", $filename >or die "Failed to open '$filename': $!"; > > Is considered preferable by most Perl programmers to: > > my $fh; > if ( not open $fh, ">", $filename ) { > die "Failed to open '$filename': $!"; > } Yeah, and that is for the same reason. When you are trying to get a birds-eye view of the codeflow, the former makes it clear that "we do something, and then we open, and then we ...", without letting the error handling (which also is rare case) distract us. > "unless" often leads to maintenance errors as the expression gets more > complicated over time,... That might also be true, but my comment was not an endorsement for (or suggestion against) use of unless. I was commenting on statement modifiers, which some people tend to overuse (or abuse) and make the resulting code harder to follow. -- 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: CodingGuidelines Perl amendment
Ted Zlatanov writes: > On Wed, 06 Feb 2013 08:29:30 -0800 Junio C Hamano wrote: > > JCH> Is it ever (as opposed to "not always") possible to omit braces? > > Oh yes! Not that I recommend it, and I'm not even going to touch on > Perl Golf :) > > JCH> It sounds as if we encourage the use of statement modifiers, which > JCH> certainly is not what I want to see. > > Yup. I think I captured that in the patch, but please feel free to > revise it after applying or throw it back to me. I'd suggest to just drop that "try to write without braces" entirely. > JCH> Incidentally, your sentence is a good example of where use of > JCH> statement modifiers is appropriate: $youmust is rarely true. > > I was trying to be funny, honestly. But OK; reworded. It wasn't a useful guidance, but it _was_ funny. -- 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: CodingGuidelines Perl amendment
On 6 February 2013 19:14, Junio C Hamano wrote: > demerphq writes: > >> As you mention below statement modifiers have their place. For instance >> >> next if $whatever; >> >> Is considered preferable to >> >> if ($whatever) { >> next; >> } >> >> Similarly >> >> open my $fh, ">", $filename >>or die "Failed to open '$filename': $!"; >> >> Is considered preferable by most Perl programmers to: >> >> my $fh; >> if ( not open $fh, ">", $filename ) { >> die "Failed to open '$filename': $!"; >> } > > Yeah, and that is for the same reason. When you are trying to get a > birds-eye view of the codeflow, the former makes it clear that "we > do something, and then we open, and then we ...", without letting > the error handling (which also is rare case) distract us. perldoc perlstyle has language which explains this well if you want to crib a description from somewhere. >> "unless" often leads to maintenance errors as the expression gets more >> complicated over time,... > > That might also be true, but my comment was not an endorsement for > (or suggestion against) use of unless. I was commenting on > statement modifiers, which some people tend to overuse (or abuse) > and make the resulting code harder to follow. That's also my point about unless. They tend to get abused and then lead to maint devs making errors, and people misunderstanding the code. The only time that unless IMO is "ok" (ish) is when it really is a very simple statement. As soon as it mentions more than one var it should be converted to an if. This applies even more so to the modifier form. Yves -- perl -Mre=debug -e "/just|another|perl|hacker/" -- 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: CodingGuidelines Perl amendment
On 6 February 2013 19:05, Ted Zlatanov wrote: > On Wed, 06 Feb 2013 08:29:30 -0800 Junio C Hamano wrote: > > JCH> Is it ever (as opposed to "not always") possible to omit braces? > > Oh yes! Not that I recommend it, and I'm not even going to touch on > Perl Golf :) I think you are wrong. Can you provide an example? Larry specifically wanted to avoid the "dangling else" problem that C suffers from, and made it so that blocks are mandatory. The only exception is statement modifiers, which are not only allowed to omit the braces but also the parens on the condition. Yves -- perl -Mre=debug -e "/just|another|perl|hacker/" -- 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: CodingGuidelines Perl amendment
On Wed, 06 Feb 2013 10:16:21 -0800 Junio C Hamano wrote: JCH> I'd suggest to just drop that "try to write without braces" entirely. OK, I'll do it on the reroll, or you can just make the change directly. I agree it was not going anywhere :) Ted diff --git a/Documentation/CodingGuidelines b/Documentation/CodingGuidelines index 951d74c..857f4e2 100644 --- a/Documentation/CodingGuidelines +++ b/Documentation/CodingGuidelines @@ -187,10 +187,6 @@ For Perl 5 programs: - use strict and use warnings are strongly preferred. - - As in C (see above), we avoid using braces unnecessarily (but Perl forces - braces around if/unless/else/foreach blocks, so this is not always possible). - At least make sure braces do not sit on their own line, like with C. - - Don't abuse statement modifiers--they are discouraged. But in general: ... do something ... -- 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
What's cooking in git.git (Feb 2013, #03; Wed, 6)
Here are the topics that have been cooking. Commits prefixed with '-' are only in 'pu' (proposed updates) while commits prefixed with '+' are in 'next'. As usual, this cycle is expected to last for 8 to 10 weeks, with a preview -rc0 sometime in the middle of this month. You can find the changes described here in the integration branches of the repositories listed at http://git-blame.blogspot.com/p/git-public-repositories.html -- [Graduated to "master"] * ft/transport-report-segv (2013-01-31) 1 commit (merged to 'next' on 2013-02-02 at 6c450a7) + push: fix segfault when HEAD points nowhere A failure to push due to non-ff while on an unborn branch dereferenced a NULL pointer when showing an error message. * jc/fake-ancestor-with-non-blobs (2013-01-31) 3 commits (merged to 'next' on 2013-02-02 at 86d457a) + apply: diagnose incomplete submodule object name better + apply: simplify build_fake_ancestor() + git-am: record full index line in the patch used while rebasing (this branch is used by jc/extended-fake-ancestor-for-gitlink.) Rebasing the history of superproject with change in the submodule has been broken since v1.7.12. * jn/auto-depend-workaround-buggy-ccache (2013-02-01) 1 commit (merged to 'next' on 2013-02-02 at db5940a) + Makefile: explicitly set target name for autogenerated dependencies An age-old workaround to prevent buggy versions of ccache from breaking the auto-generation of dependencies, which unfortunately is still relevant because some people use ancient distros. * sb/gpg-plug-fd-leak (2013-01-31) 1 commit (merged to 'next' on 2013-02-02 at c271a31) + gpg: close stderr once finished with it in verify_signed_buffer() We forgot to close the file descriptor reading from "gpg" output, killing "git log --show-signature" on a long history. * ta/doc-no-small-caps (2013-02-01) 6 commits (merged to 'next' on 2013-02-02 at 77cbd0e) + Documentation: StGit is the right spelling, not StGIT + Documentation: describe the "repository" in repository-layout + Documentation: add a description for 'gitfile' to glossary + Documentation: do not use undefined terms git-dir and git-file + Documentation: the name of the system is 'Git', not 'git' + Documentation: avoid poor-man's small caps GIT Update documentation to change "GIT" which was a poor-man's small caps to "Git". The latter was the intended spelling. Also change "git" spelled in all-lowercase to "Git" when it refers to the system as the whole or the concept it embodies, as opposed to the command the end users would type. -- [New Topics] * jc/extended-fake-ancestor-for-gitlink (2013-02-05) 1 commit - apply: verify submodule commit object name better Instead of requiring the full 40-hex object names on the index line, we can read submodule commit object names from the textual diff when synthesizing a fake ancestore tree for "git am -3". Will merge to 'next'. * tz/credential-authinfo (2013-02-05) 1 commit - Add contrib/credentials/netrc with GPG support A new read-only credential helper (in contrib/) to interact with the .netrc/.authinfo files. Hopefully mn/send-email-authinfo topic can rebuild on top of something like this. Waiting for further reviews. * jx/utf8-printf-width (2013-02-05) 1 commit - Add utf8_fprintf helper which returns correct columns Use a new helper that prints a message and counts its display width to align the help messages parse-options produces. -- [Stalled] * mn/send-email-authinfo (2013-01-29) 1 commit - git-send-email: add ~/.authinfo parsing This triggered a subtopic to add a credential helper for authinfo/netrc files (with or without GPG encryption), but nobody seems to be working on connecting send-email to the credential framework. * mp/diff-algo-config (2013-01-16) 3 commits - diff: Introduce --diff-algorithm command line option - config: Introduce diff.algorithm variable - git-completion.bash: Autocomplete --minimal and --histogram for git-diff Add diff.algorithm configuration so that the user does not type "diff --histogram". Looking better; may want tests to protect it from future breakages, but otherwise it looks ready for 'next'. Expecting a follow-up to add tests. * mb/gitweb-highlight-link-target (2012-12-20) 1 commit - Highlight the link target line in Gitweb using CSS Expecting a reroll. $gmane/211935 * jk/lua-hackery (2012-10-07) 6 commits - pretty: fix up one-off format_commit_message calls - Minimum compilation fixup - Makefile: make "lua" a bit more configurable - add a "lua" pretty format - add basic lua infrastructure - pretty: make some commit-parsing helpers more public Interesting exercise. When we do this for real, we probably would want to wrap a commit to make it more like an "object" with methods like "parents", etc. * rc/maint-complete-
Re: Bug in "git log --graph -p -m" (version 1.7.7.6)
John Keeping writes: > I would argue that the line should start with "| | ", since it really is > just a continuation of the same commit. > > | | > | | commit a393ed598e9fb11436f85bd58f1a38c82f2cadb7 (from > 33e70e70c0173d634826b998bdc304f93c0966b8) > | | Merge: 2c1e6a3 33e70e7 > | | Author: Matthieu Moy > | | Date: Tue Feb 5 22:05:33 2013 +0100 Yes. I had a look at the code, I guess the call to graph_show_commit() in show_log() (in log-tree.c) should have called graph_show_padding() but didn't in this case. Then I got lost in graph.c :-(. -- Matthieu Moy http://www-verimag.imag.fr/~moy/ -- 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: CodingGuidelines Perl amendment
On Wed, 6 Feb 2013 19:25:43 +0100 demerphq wrote: d> On 6 February 2013 19:05, Ted Zlatanov wrote: >> On Wed, 06 Feb 2013 08:29:30 -0800 Junio C Hamano wrote: >> JCH> Is it ever (as opposed to "not always") possible to omit braces? >> >> Oh yes! Not that I recommend it, and I'm not even going to touch on >> Perl Golf :) d> I think you are wrong. Can you provide an example? d> Larry specifically wanted to avoid the "dangling else" problem that C d> suffers from, and made it so that blocks are mandatory. The only d> exception is statement modifiers, which are not only allowed to omit d> the braces but also the parens on the condition. Oh, perhaps I didn't state it correctly. You can avoid braces, but not if you want to use if/elsif/else/unless/etc. which require them: condition && do_this(); condition || do_this(); condition ? do_this() : do_that(); (and others I can't recall right now) But my point was only that it's always possible to get around these artificial restrictions; it's more important to ask for legible sensible code. Sorry if that was unclear! Ted -- 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: CodingGuidelines Perl amendment
On 6 February 2013 19:35, Ted Zlatanov wrote: > On Wed, 6 Feb 2013 19:25:43 +0100 demerphq wrote: > > d> On 6 February 2013 19:05, Ted Zlatanov wrote: >>> On Wed, 06 Feb 2013 08:29:30 -0800 Junio C Hamano wrote: >>> > JCH> Is it ever (as opposed to "not always") possible to omit braces? >>> >>> Oh yes! Not that I recommend it, and I'm not even going to touch on >>> Perl Golf :) > > d> I think you are wrong. Can you provide an example? > > d> Larry specifically wanted to avoid the "dangling else" problem that C > d> suffers from, and made it so that blocks are mandatory. The only > d> exception is statement modifiers, which are not only allowed to omit > d> the braces but also the parens on the condition. > > Oh, perhaps I didn't state it correctly. You can avoid braces, but not > if you want to use if/elsif/else/unless/etc. which require them: > > condition && do_this(); > condition || do_this(); > condition ? do_this() : do_that(); > > (and others I can't recall right now) > > But my point was only that it's always possible to get around these > artificial restrictions; it's more important to ask for legible sensible > code. Sorry if that was unclear! Ah ok. Right, at a low level: if (condition) { do_this() } is identical to condition && do_this(); IOW, Perl allows logical operators to act as control flow statements. I hope your document include something that says that using logical operators as control flow statements should be used sparingly, and generally should be restricted to low precedence operators and should never involve more than one operator. Yves -- perl -Mre=debug -e "/just|another|perl|hacker/" -- 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: CodingGuidelines Perl amendment
On Wed, 6 Feb 2013 19:44:16 +0100 demerphq wrote: d> Ah ok. Right, at a low level: d> if (condition) { do_this() } d> is identical to d> condition && do_this(); d> IOW, Perl allows logical operators to act as control flow statements. d> I hope your document include something that says that using logical d> operators as control flow statements should be used sparingly, and d> generally should be restricted to low precedence operators and should d> never involve more than one operator. I'd stay away from wording it so tightly, but instead just say "Make your code readable and sensible, and don't try to be clever." But this is good C and shell advice too, so I'd put it under "General Guidelines" and leave it for Junio to decide if it's appropriate. Ted -- 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 v3 0/8] Hiding refs
Duy Nguyen writes: > On Tue, Feb 5, 2013 at 5:29 PM, Michael Haggerty wrote: >> Hiderefs creates a "dark" corner of a remote git repo that can hold >> arbitrary content that is impossible for anybody to discover but >> nevertheless possible for anybody to download (if they know the name of >> a hidden reference). In earlier versions of the patch series I believe >> that it was possible to push to a hidden reference hierarchy, which made >> it possible to upload dark content. The new version appears (from the >> code) to prohibit adding references in a hidden hierarchy, which would >> close the main loophole that I was worried about. But the documentation >> and the unit tests only explicitly say that updates and deletes are >> prohibited; nothing is said about adding references (unless "update" is >> understood to include "add"). I think the true behavior should be >> clarified and tested. >> >> I was worried that somehow this "dark" content could be used for >> malicious purposes; for example, pushing compromised code then >> convincing somebody to download it by SHA1 with the implicit argument >> "it's safe since it comes directly from the project's official >> repository". If it is indeed impossible to populate the dark namespace >> remotely then I can't think of a way to exploit it. > > Or you can think hiderefs is the first step to addressing the > initial ref advertisment problem. The series says hidden refs are > to be fetched out of band, but that's not the only way. Let me help unconfuse this thread. I think the series as 8-patch series was poorly presented, and separating it into two will help understanding what they are about. The first three: upload-pack: share more code upload-pack: simplify request validation upload/receive-pack: allow hiding ref hierarchies is _the_ topic of the series. As far as I am concerned (I am not speaking for Gerrit users, but am speaking as the Git maintainer), the topic is solely about uncluttering. There may be refs that the server end may need to keep for its operation, but that remote users have _no_ business knowing about. Allowing the server to keep these refs in the repository, while not showing these refs over the wire, is the problem the series solves. In other words, it is not about "these are *usually* not wanted by clients, so do not show them by default". It is about "these are not to be shown, ever". OK? Now, there may be some refs that are not *usually* wanted by clients but there may be cases where clients want to (1) learn about them via the same protocol; and/or (2) fetch them over the protocol. If you want to solve both of these two issues generally, the solution has to involve a separate protocol from the today's protocol. It would go like this: * The upload-pack-2 service sits on a port different from today's, waits for a ls-remote/fetch/clone client to connect to it, makes a default advertisement that only includes the refs that are usually wanted by clients with hints on what other refs the initial advertisement omitted, to let the client know that it is allowed to ask for them. * An updated client, if it sees that some refs are omitted from the initial advertisement *and* what the user told it to fetch or list may be one of the omitted ones (this is why the server gives hints in the previous step in the first step; when the server says it did not omit anything, or when it says it omitted only refs/pull/*, a client that wanted to fetch refs/heads/frotz will know the request will fail without continuing this step), then makes a "expand-refs" request to the server, asking for the refs it did not see and the server could supply. * When the server sees "expand-refs", it responds with additional advertisement. "expand-refs refs/pull/*" may result in listing of all refs in that hierarchy. "expand-refs refs/changes/1/1" would result in listing that single ref. "expand-refs no-such" may result in nothing, indicating an error. * After the (possible) expand-refs exchange, the client knows exactly the same and necessary information as the current protocol gives it in order to go to the common ancestor discovery step, and the protocol can continue the same way as the current protocol. Note that this cannot sit on the current port in general, as existing clients will not be able to tell some refs are not advertised, so unless you are hiding large and truly unused part of the refspace, interoperability with older clients will render the mechanism useless. You cannot use this to delay the refs/tags/ hierarchy with this mechanism and have older client come to the updated service that by default does not advertise tags, for example. The above is what I called the "delayed advertisement" in the discussion, which was brought up several months ago but nothing materialized as the result. People who are interested in pursuing this can volunteer and start di
Re: What's cooking in git.git (Feb 2013, #03; Wed, 6)
Am 06.02.2013 19:29, schrieb Junio C Hamano: > * jl/submodule-deinit (2013-02-04) 1 commit > - submodule: add 'deinit' command > > There was no Porcelain way to say "I no longer am interested in > this submodule", once you express your interest in a submodule with > "submodule init". "submodule deinit" is the way to do so. > > Will merge to 'next'. Oops, I though you were waiting for a reroll. Currently I'm having the appended interdiff compared to your version. Changes are: - Add deinit to the --force documentation of "git submodule" - Never remove submodules containing a .git dir, even when forced - diagnostic output when "rm -rf" or "mkdir" fails - More test cases And I wanted to add three more test cases for modified submodules before sending v4. You could squash in the first two hunks into the commit you have in pu and I'll send a follow up patch with the extra tests soon or you could wait for me sending an updated patch. What do you think? ---8<-- diff --git a/Documentation/git-submodule.txt b/Documentation/git-submodule.txt index 7a149eb..45ee12b 100644 --- a/Documentation/git-submodule.txt +++ b/Documentation/git-submodule.txt @@ -227,8 +227,10 @@ OPTIONS -f:: --force:: - This option is only valid for add and update commands. + This option is only valid for add, deinit and update commands. When running add, allow adding an otherwise ignored submodule path. + When running deinit the submodule work trees will be removed even if + they contain local changes. When running update, throw away local changes in submodules when switching to a different commit; and always run a checkout operation in the submodule, even if the commit listed in the index of the diff --git a/git-submodule.sh b/git-submodule.sh index f05b597..365c6de 100755 --- a/git-submodule.sh +++ b/git-submodule.sh @@ -595,14 +595,25 @@ cmd_deinit() continue fi - # Remove the submodule work tree - if test -z "$force" + # Remove the submodule work tree (unless the user already did it) + if test -d "$sm_path" then - git rm -n "$sm_path" || - die "$(eval_gettext "Submodule work tree $sm_path contains local modifications, use '-f' to discard them")" + # Protect submodules containing a .git directory + if test -d "$sm_path/.git" + then + echo >&2 "$(eval_gettext "Submodule work tree $sm_path contains a .git directory")" + die "$(eval_gettext "(use 'rm -rf' if you really want to remove it including all of its history)")" + fi + + if test -z "$force" + then + git rm -n "$sm_path" || + die "$(eval_gettext "Submodule work tree $sm_path contains local modifications, use '-f' to discard them")" + fi + rm -rf "$sm_path" || say "$(eval_gettext "Could not remove submodule work tree '\$sm_path'")" fi - rm -rf "$sm_path" - mkdir "$sm_path" + + mkdir "$sm_path" || say "$(eval_gettext "Could not create empty submodule directory '\$sm_path'")" # Remove the whole section so we have a clean state when the # user later decides to init this submodule again diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh index 34d8274..0567f1a 100755 --- a/t/t7400-submodule-basic.sh +++ b/t/t7400-submodule-basic.sh @@ -757,20 +757,46 @@ test_expect_success 'submodule add with an existing name fails unless forced' ' ) ' +test_expect_success 'set up a second submodule' ' + git submodule add ./init2 example2 && + git commit -m "submodle example2 added" +' + test_expect_success 'submodule deinit should remove the whole submodule section from .git/config' ' git config submodule.example.foo bar && + git config submodule.example2.frotz nitfol && git submodule deinit init && test -z "$(git config submodule.example.url)" && - test -z "$(git config submodule.example.foo)" + test -z "$(git config submodule.example.foo)" && + test -n "$(git config submodule.example2.url)" && + test -n "$(git config submodule.example2.frotz)" && + rmdir init ' test_expect_success 'submodule deinit . deinits all initialized submodules' ' git submodule update --init && git config submodule.example.foo bar && + git config submodule.example2.frotz nitfol && test_must_fail git submodule deinit && git submodule deinit . && test -z "$(git config submodule.example.url)" && - test -z "$(git config submodule.example.foo)" +
Re: CodingGuidelines Perl amendment
Ted Zlatanov writes: > "Make your code readable and sensible, and don't try to be clever." > > But this is good C and shell advice too,... Sounds sensible. -- 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: What's cooking in git.git (Feb 2013, #03; Wed, 6)
Jens Lehmann writes: > Am 06.02.2013 19:29, schrieb Junio C Hamano: >> * jl/submodule-deinit (2013-02-04) 1 commit >> - submodule: add 'deinit' command >> >> There was no Porcelain way to say "I no longer am interested in >> this submodule", once you express your interest in a submodule with >> "submodule init". "submodule deinit" is the way to do so. >> >> Will merge to 'next'. > > Oops, I though you were waiting for a reroll. Currently I'm having the > appended interdiff compared to your version. Changes are: > > - Add deinit to the --force documentation of "git submodule" > - Never remove submodules containing a .git dir, even when forced > - diagnostic output when "rm -rf" or "mkdir" fails > - More test cases > > And I wanted to add three more test cases for modified submodules before > sending v4. You could squash in the first two hunks into the commit you > have in pu and I'll send a follow up patch with the extra tests soon or > you could wait for me sending an updated patch. What do you think? I haven't merged it down to 'next' yet. So please proceed as you planned. Thanks for stopping 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
Re: [PATCH v3 0/8] Hiding refs
Junio C Hamano wrote: > Duy Nguyen writes: >> On Tue, Feb 5, 2013 at 5:29 PM, Michael Haggerty >> wrote: >>> Hiderefs creates a "dark" corner of a remote git repo [...] >> Or you can think hiderefs is the first step to addressing the >> initial ref advertisment problem. The series says hidden refs are >> to be fetched out of band, but that's not the only way. > > Let me help unconfuse this thread. > > I think the series as 8-patch series was poorly presented, and > separating it into two will help understanding what they are about. > > The first three: > > upload-pack: share more code > upload-pack: simplify request validation > upload/receive-pack: allow hiding ref hierarchies > > is _the_ topic of the series. As far as I am concerned (I am not > speaking for Gerrit users, but am speaking as the Git maintainer), > the topic is solely about uncluttering. There may be refs that the > server end may need to keep for its operation, but that remote users > have _no_ business knowing about. An obvious question when looking at that alone is, is there ever actually need for such private refs? If the refs are not meant to be shared with users *at all*, why are they even refs? An answer is "because refs force gc to keep the corresponding objects". For example, the sysadmin may want to keep refs/archived/ refs for dead branches that should not be advertised or accessible to the user any more. Seems sane, though not especially exciting. What is more exciting to me is that it is a first step toward addressing the complicated problem of offering access to more refs than can be efficiently presented in the current ref advertisement. I think that's a harder problem but something like this would be needed in order to support existing clients without performance degredation. And in the meantime, it helps with the refs/archived case. Thanks for explaining. Jonathan -- 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 v2] Update CodingGuidelines for Perl 5
Update the coding guidelines for Perl 5. Signed-off-by: Ted Zlatanov --- Changes since PATCHv1: - removed brace guidelines - add "don't try to be clever" at beginning Documentation/CodingGuidelines | 42 1 files changed, 42 insertions(+), 0 deletions(-) diff --git a/Documentation/CodingGuidelines b/Documentation/CodingGuidelines index 1d7de5f..166c141 100644 --- a/Documentation/CodingGuidelines +++ b/Documentation/CodingGuidelines @@ -18,6 +18,8 @@ code. For Git in general, three rough rules are: judgement call, the decision based more on real world constraints people face than what the paper standard says. +For any programming language below, make your code readable and sensible, and +don't try to be clever. As for more concrete guidelines, just imitate the existing code (this is a good guideline, no matter which project you are @@ -179,6 +181,46 @@ For C programs: - Use Git's gettext wrappers to make the user interface translatable. See "Marking strings for translation" in po/README. +For Perl 5 programs: + + - Most of the C guidelines above apply. + + - We try to support Perl 5.8 and later ("use Perl 5.008"). + + - use strict and use warnings are strongly preferred. + + - Don't abuse statement modifiers--they are discouraged. But in general: + + ... do something ... + do_this() unless (condition); +... do something else ... + + should be used instead of + + ... do something ... + unless (condition) { + do_this(); + } +... do something else ... + + *only* when when the condition is so rare that do_this() will be called + almost always. + + - We try to avoid assignments inside if(). + + - Learn and use Git.pm if you need that functionality. + + - For Emacs, it's useful to put the following in + GIT_CHECKOUT/.dir-locals.el, assuming you use cperl-mode: + +;; note the first part is useful for C editing, too +((nil . ((indent-tabs-mode . t) + (tab-width . 8) + (fill-column . 80))) + (cperl-mode . ((cperl-indent-level . 8) +(cperl-extra-newline-before-brace . nil) +(cperl-merge-trailing-else . t + Writing Documentation: Every user-visible change should be reflected in the documentation. -- 1.7.9.rc2 -- 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 v3 0/8] Hiding refs
Michael Haggerty wrote: > Scenario 1: Some providers junk up their users' repositories with > content that is not created by the repository's owner and that the owner > doesn't want to appear to vouch for (e.g., GitHub pull requests). These > references might sometimes be useful to fetch, singly or in bulk. > > Scenario 2: Some systems junk up their users' repositories with > additional references that are not interesting to most pullers (e.g., > Gerrit activity markers) though they don't add questionable content. Actually Gerrit's refs/changes refs are pretty similar to Github's refs/pull. Both are requests for code review. [...] > But now every time I do a "gitk --all" or "git log --decorate", the > output is cluttered with all of his references (most of which are just > old versions of references from the upstream repository that we both > use). I would like to be able to hide his references most of the time > but turn them back on when I need them. > > Scenario 5: Our upstream repository has gazillions of release tags under > "refs/tags/releases/...", sometimes including customer-specific > releases. In my daily life these are just clutter. For both of these use cases, putting the refs somewhere other than refs/heads, refs/tags, and refs/remotes should be enough to avoid clutter. I agree that a --decorate-glob along the lines of "git rev-parse"'s --glob would be nice. [...] > * Some small improvements (e.g. allowing *multiple* views to be > defined) would provide much more benefit for about the same effort, > and would be a better base for building other features in the future > (e.g., local views). Would advertising GIT_CONFIG_PARAMETERS and giving examples for server admins to set it in inetd et al to provide different kinds of access to a same repository through different URLs work? > Thanks for listening. > Michael > > [1] Theoretically one could support multiple views of a single > repository by using something like "GIT_CONFIG=view_1_config git > upload-pack ..." or "git -c transfer.hiderefs=... git upload-pack ...", > but this would be awkward. Ah, I missed this comment before. What's awkward about that? I think it's a clean way to make many aspects of how a repository is presented (including hook actions) configurable. Thanks for your help clarifying this feature. Hopefully some of the discussion will filter into the documentation. Jonathan -- 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] graph: output padding for merge subsequent parents
On Wed, Feb 06, 2013 at 07:33:08PM +0100, Matthieu Moy wrote: > John Keeping writes: > > > I would argue that the line should start with "| | ", since it really is > > just a continuation of the same commit. > > > > | | > > | | commit a393ed598e9fb11436f85bd58f1a38c82f2cadb7 (from > > 33e70e70c0173d634826b998bdc304f93c0966b8) > > | | Merge: 2c1e6a3 33e70e7 > > | | Author: Matthieu Moy > > | | Date: Tue Feb 5 22:05:33 2013 +0100 > > Yes. > > I had a look at the code, I guess the call to graph_show_commit() in > show_log() (in log-tree.c) should have called graph_show_padding() but > didn't in this case. Then I got lost in graph.c :-(. I think this is the correct answer. But now I've found that "git log --graph -c -p" doesn't indent the diff - that seems to be a separate issue. -- >8 -- When showing merges in git-log, the same commit is shown once for each parent. Combined with "--graph" this results in graph_show_commit() being called once for each parent without graph_update() being called. Currently graph_show_commit() does not print anything on subsequent invocations for the same commit (this was changed by commit 656197a - "graph.c: infinite loop in git whatchanged --graph -m" from the previous behaviour of looping infinitely). Change this so that if the graph code believes it has already shown the commit it prints a single padding line. Signed-off-by: John Keeping --- graph.c | 10 ++ 1 file changed, 10 insertions(+) diff --git a/graph.c b/graph.c index 391a712..2a3fc5c 100644 --- a/graph.c +++ b/graph.c @@ -1227,6 +1227,16 @@ void graph_show_commit(struct git_graph *graph) if (!graph) return; + /* +* When showing a diff of a merge against each of its parents, we +* are called once for each parent without graph_update having been +* called. In this case, simply output a single padding line. +*/ + if (graph_is_commit_finished(graph)) { + graph_show_padding(graph); + shown_commit_line = 1; + } + while (!shown_commit_line && !graph_is_commit_finished(graph)) { shown_commit_line = graph_next_line(graph, &msgbuf); fwrite(msgbuf.buf, sizeof(char), msgbuf.len, stdout); -- 1.8.1.2 -- 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: [RFC] test-lib.sh: No POSIXPERM for cygwin
Am 2013-02-06 10:34, schrieb Erik Faye-Lund: On Sun, Jan 27, 2013 at 3:57 PM, Torsten Bögershausen wrote: t0070 and t1301 fail when running the test suite under cygwin. Skip the failing tests by unsetting POSIXPERM. But is this the real reason? I thought Cygwin implemented POSIX permissions...? t0070: 'mktemp to unwritable directory prints filename' mkdir cannotwrite && chmod -w cannotwrite && test_when_finished "chmod +w cannotwrite" && test_must_fail test-mktemp cannotwrite/testXX 2>err && grep "cannotwrite/test" err When a directory under Linux/*nix has no write permission, it is not allowed to create another directory (or file..) here. This is not working under cygwin, a directory/file can be created even if the parent directory has chmod 0. - tb@PC /cygdrive/c/temp $ mkdir ttt tb@PC /cygdrive/c/temp $ chmod 0 ttt tb@PC /cygdrive/c/temp $ ls -ld ttt d-+ 1 tb None 0 Feb 6 20:33 ttt tb@PC /cygdrive/c/temp $ touch ttt/x tb@PC /cygdrive/c/temp $ ls -ld ttt d-+ 1 tb None 0 Feb 6 20:33 ttt tb@PC /cygdrive/c/temp $ ls -l ttt total 0 -rw-r--r--+ 1 tb None 0 Feb 6 20:33 x --- If this is POSIX compliant? I'm not an expert here. On the other hand: This test case does not test git, but rather the file system, so we can probaly remove it? About 1301: Some resereach needs to be done, to find out the connection between umask, cygwin and the mount options. On my system I have: $mount C: on /cygdrive/c type ntfs (binary,posix=0,user,noumount,auto) /Torsten -- 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 0/4] Make git-send-email git-credential
From: Michal Nazarewicz As discussed on the list, adding git-credential interface to Git.pm (sort of copied from git-remote-mediawiki) and making git-send-email use it. I see git-remote-mediawiki does not have “use Git” so I did not touch it. On top of that I'd have no way to tests the changes anyway. Michal Nazarewicz (4): Git.pm: Allow command_close_bidi_pipe() to be called as method Git.pm: Allow pipes to be closed prior to calling command_close_bidi_pipe Git.pm: Add interface for git credential command. git-send-email: Use git credential to obtain password. Documentation/git-send-email.txt | 4 +- git-send-email.perl | 60 +++- perl/Git.pm | 116 ++- 3 files changed, 149 insertions(+), 31 deletions(-) -- 1.8.1.2.550.g0d3a9c0.dirty -- 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 0/4] Make git-send-email git-credential
On Wed, Feb 06 2013, Michal Nazarewicz wrote: > As discussed on the list, adding git-credential interface to Git.pm > (sort of copied from git-remote-mediawiki) and making git-send-email > use it. > > I see git-remote-mediawiki does not have “use Git” so I did not touch > it. On top of that I'd have no way to tests the changes anyway. > > Michal Nazarewicz (4): > Git.pm: Allow command_close_bidi_pipe() to be called as method > Git.pm: Allow pipes to be closed prior to calling > command_close_bidi_pipe > Git.pm: Add interface for git credential command. > git-send-email: Use git credential to obtain password. > > Documentation/git-send-email.txt | 4 +- > git-send-email.perl | 60 +++- > perl/Git.pm | 116 > ++- > 3 files changed, 149 insertions(+), 31 deletions(-) On second thought, give me a moment, ;) I've just discovered a bug preventing git-send-email from mailing a patchset. -- Best regards, _ _ .o. | Liege of Serenely Enlightened Majesty of o' \,=./ `o ..o | Computer Science, Michał “mina86” Nazarewicz(o o) ooo +--ooO--(_)--Ooo-- pgpHpgySqvgfA.pgp Description: PGP signature
Re: Why is ident_is_sufficient different on Windows?
Max Horn writes: > static int ident_is_sufficient(int user_ident_explicitly_given) > { > #ifndef WINDOWS > return (user_ident_explicitly_given & IDENT_MAIL_GIVEN); > #else > return (user_ident_explicitly_given == IDENT_ALL_GIVEN); > #endif > } > > > According to git blame, this was introduced here: > > commit 5aeb3a3a838b2cb03d250f3376cf9c41f4d4608e > Author: Junio C Hamano > Date: Sun Jan 17 13:54:28 2010 -0800 > > user_ident_sufficiently_given(): refactor the logic to be usable from > elsewhere > > > The commit message sounds as if this was only a refactoring, but > the patch to me look as if it changes behaviour, too. Of course > this could very well be false, say due to code elsewhere that > already caused Windows to behave differently; I wouldn't know. > > Still, I wonder: Why does this difference exist? Sorry but I do not recall why these ifdefs are there. The commit did this to builtin-commit.c: - if (user_ident_explicitly_given != IDENT_ALL_GIVEN) + if (!user_ident_sufficiently_given()) I would have written the function to always check with ALL_GIVEN myself, and it is very likely that I was *not* the person who noticed that the function needs to behave differently on Windows, as I do not do Windows. I suspect somebody from the Windows camp saw a patch I posted without the ifdef, noticed that there is a problem to expect IDENT_NAME_GIVEN to be set on Windows for some reason, and resulted in a reroll of the function in that shape. I didn't find anything in the list archive, though. So I am stumped. -- 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 0/4] Make git-send-email git-credential
From: Michal Nazarewicz As discussed on the list, adding git-credential interface to Git.pm (sort of copied from git-remote-mediawiki) and making git-send-email use it. I see git-remote-mediawiki does not have “use Git” so I did not touch it. On top of that I'd have no way to tests the changes anyway. Michal Nazarewicz (4): Git.pm: Allow command_close_bidi_pipe() to be called as method Git.pm: Allow pipes to be closed prior to calling command_close_bidi_pipe Git.pm: Add interface for git credential command. git-send-email: Use git credential to obtain password. Documentation/git-send-email.txt | 4 +- git-send-email.perl | 59 +++- perl/Git.pm | 116 ++- 3 files changed, 149 insertions(+), 30 deletions(-) -- 1.8.1.2.549.g4fa355e -- 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 1/4] Git.pm: Allow command_close_bidi_pipe() to be called as method
From: Michal Nazarewicz The documentation of command_close_bidi_pipe() claims that it can be called as a method, but it does not check whether the first argument is $self or not assuming the latter. Using _maybe_self() fixes this. Signed-off-by: Michal Nazarewicz --- perl/Git.pm | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/perl/Git.pm b/perl/Git.pm index 931047c..bbb753a 100644 --- a/perl/Git.pm +++ b/perl/Git.pm @@ -430,7 +430,7 @@ have more complicated structure. sub command_close_bidi_pipe { local $?; - my ($pid, $in, $out, $ctx) = @_; + my ($self, $pid, $in, $out, $ctx) = _maybe_self(@_); foreach my $fh ($in, $out) { unless (close $fh) { if ($!) { -- 1.8.1.2.549.g4fa355e -- 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 2/4] Git.pm: Allow pipes to be closed prior to calling command_close_bidi_pipe
From: Michal Nazarewicz The command_close_bidi_pipe() function will insist on closing both input and output pipes returned by command_bidi_pipe(). With this change it is possible to close one of the pipes in advance and pass undef as an argument. This allows for something like: my ($pid, $in, $out, $ctx) = command_bidi_pipe(...); print $out "write data"; close $out; # ... do stuff with $in command_close_bidi_pipe($pid, $in, undef, $ctx); Signed-off-by: Michal Nazarewicz --- perl/Git.pm | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/perl/Git.pm b/perl/Git.pm index bbb753a..6a2d52d 100644 --- a/perl/Git.pm +++ b/perl/Git.pm @@ -432,7 +432,7 @@ sub command_close_bidi_pipe { local $?; my ($self, $pid, $in, $out, $ctx) = _maybe_self(@_); foreach my $fh ($in, $out) { - unless (close $fh) { + if (defined $fh && !close $fh) { if ($!) { carp "error closing pipe: $!"; } elsif ($? >> 8) { -- 1.8.1.2.549.g4fa355e -- 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 3/4] Git.pm: Add interface for git credential command.
From: Michal Nazarewicz Add a credential() function which is an interface to the git credential command. Signed-off-by: Michal Nazarewicz --- perl/Git.pm | 112 +++- 1 file changed, 111 insertions(+), 1 deletion(-) diff --git a/perl/Git.pm b/perl/Git.pm index 6a2d52d..5a18921 100644 --- a/perl/Git.pm +++ b/perl/Git.pm @@ -59,7 +59,8 @@ require Exporter; command_bidi_pipe command_close_bidi_pipe version exec_path html_path hash_object git_cmd_try remote_refs prompt -temp_acquire temp_release temp_reset temp_path); +temp_acquire temp_release temp_reset temp_path +credential); =head1 DESCRIPTION @@ -1000,6 +1001,115 @@ sub _close_cat_blob { } +sub _credential_read { + my %credential; + my ($reader, $op) = (@_); + while (<$reader>) { + chomp; + my ($key, $value) = /([^=]*)=(.*)/; + if (not defined $key) { + throw Error::Simple("unable to parse git credential $op response:\n$_\n"); + } + $credential{$key} = $value; + } + return %credential; +} + +sub _credential_write { + my ($credential, $writer) = @_; + + for my $key (sort { + # url overwrites other fields, so it must come first + return -1 if $a eq 'url'; + return 1 if $b eq 'url'; + return $a cmp $b; + } keys %$credential) { + if (defined $credential->{$key} && length $credential->{$key}) { + print $writer $key, '=', $credential->{$key}, "\n"; + } + } + print $writer "\n"; +} + +sub _credential_run { + my ($self, $credential, $op) = _maybe_self(@_); + + my ($pid, $reader, $writer, $ctx) = command_bidi_pipe('credential', $op); + + _credential_write $credential, $writer; + close $writer; + + if ($op eq "fill") { + %$credential = _credential_read $reader, $op; + } elsif (<$reader>) { + throw Error::Simple("unexpected output from git credential $op response:\n$_\n"); + } + + command_close_bidi_pipe($pid, $reader, undef, $ctx); +} + +=item credential( CREDENTIAL_HASH [, OPERATION ] ) + +=item credential( CREDENTIAL_HASH, CODE ) + +Executes C for a given set of credentials and +specified operation. In both form C needs to be +a reference to a hash which stores credentials. Under certain +conditions the hash can change. + +In the first form, C can be C<'fill'> (or omitted), +C<'approve'> or C<'reject'>, and function will execute corresponding +C sub-command. In case of C<'fill'> the values stored +in C will be changed to the ones returned by the +C command. The usual usage would look something like: + + my %cred = ( + 'protocol' => 'https', + 'host' => 'example.com', + 'username' => 'bob' + ); + Git::credential \%cred; + if (try_to_authenticate($cred{'username'}, $cred{'password'})) { + Git::credential \%cred, 'approve'; + ... do more stuff ... + } else { + Git::credential \%cred, 'reject'; + } + +In the second form, C needs to be a reference to a subroutine. +The function will execute C to fill provided +credential hash, than call C with C as the sole +argument, and finally depending on C's return value execute +C (if return value yields true) or C (otherwise). The return value is the same as what +C returned. With this form, the usage might look as follows: + + if (Git::credential { + 'protocol' => 'https', + 'host' => 'example.com', + 'username' => 'bob' + }, sub { + my $cred = shift; + return try_to_authenticate($cred->{'username'}, $cred->{'password'}); + }) { + ... do more stuff ... + } + +=cut + +sub credential { + my ($self, $credential, $op_or_code) = (_maybe_self(@_), 'fill'); + + if ('CODE' eq ref $op_or_code) { + _credential_run $credential, 'fill'; + my $ret = $op_or_code->($credential); + _credential_run $credential, $ret ? 'approve' : 'reject'; + return $ret; + } else { + _credential_run $credential, $op_or_code; + } +} + { # %TEMP_* Lexical Context my (%TEMP_FILEMAP, %TEMP_FILES); -- 1.8.1.2.549.g4fa355e -- 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 4/4] git-send-email: Use git credential to obtain password.
From: Michal Nazarewicz If smtp_user is provided but smtp_pass is not, instead of prompting for password, make git-send-email use git credential command instead. Signed-off-by: Michal Nazarewicz --- Documentation/git-send-email.txt | 4 +-- git-send-email.perl | 59 +++- 2 files changed, 36 insertions(+), 27 deletions(-) diff --git a/Documentation/git-send-email.txt b/Documentation/git-send-email.txt index 44a1f7c..0cffef8 100644 --- a/Documentation/git-send-email.txt +++ b/Documentation/git-send-email.txt @@ -164,8 +164,8 @@ Sending Furthermore, passwords need not be specified in configuration files or on the command line. If a username has been specified (with '--smtp-user' or a 'sendemail.smtpuser'), but no password has been -specified (with '--smtp-pass' or 'sendemail.smtppass'), then the -user is prompted for a password while the input is masked for privacy. +specified (with '--smtp-pass' or 'sendemail.smtppass'), then +a password is obtained using 'git-credential'. --smtp-server=:: If set, specifies the outgoing SMTP server to use (e.g. diff --git a/git-send-email.perl b/git-send-email.perl index be809e5..76bbfc3 100755 --- a/git-send-email.perl +++ b/git-send-email.perl @@ -1045,6 +1045,39 @@ sub maildomain { return maildomain_net() || maildomain_mta() || 'localhost.localdomain'; } +# Returns 1 if authentication succeeded or was not necessary +# (smtp_user was not specified), and 0 otherwise. + +sub smtp_auth_maybe { + if (!defined $smtp_authuser || $auth) { + return 1; + } + + # Workaround AUTH PLAIN/LOGIN interaction defect + # with Authen::SASL::Cyrus + eval { + require Authen::SASL; + Authen::SASL->import(qw(Perl)); + }; + + # TODO: Authentication may fail not because credentials were + # invalid but due to other reasons, in which we should not + # reject credentials. + $auth = Git::credential({ + 'protocol' => 'smtp', + 'host' => join(':', $smtp_server, $smtp_server_port), + 'username' => $smtp_authuser, + # if there's no password, "git credential fill" will + # give us one, otherwise it'll just pass this one. + 'password' => $smtp_authpass + }, sub { + my $cred = shift; + return !!$smtp->auth($cred->{'username'}, $cred->{'password'}); + }); + + return $auth; +} + # Returns 1 if the message was sent, and 0 otherwise. # In actuality, the whole program dies when there # is an error sending a message. @@ -1185,31 +1218,7 @@ X-Mailer: git-send-email $gitversion defined $smtp_server_port ? " port=$smtp_server_port" : ""; } - if (defined $smtp_authuser) { - # Workaround AUTH PLAIN/LOGIN interaction defect - # with Authen::SASL::Cyrus - eval { - require Authen::SASL; - Authen::SASL->import(qw(Perl)); - }; - - if (!defined $smtp_authpass) { - - system "stty -echo"; - - do { - print "Password: "; - $_ = ; - print "\n"; - } while (!defined $_); - - chomp($smtp_authpass = $_); - - system "stty echo"; - } - - $auth ||= $smtp->auth( $smtp_authuser, $smtp_authpass ) or die $smtp->message; - } + smtp_auth_maybe or die $smtp->message; $smtp->mail( $raw_from ) or die $smtp->message; $smtp->to( @recipients ) or die $smtp->message; -- 1.8.1.2.549.g4fa355e -- 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: Why is ident_is_sufficient different on Windows?
Junio C Hamano writes: > I suspect somebody from the Windows camp saw a patch I posted > without the ifdef, noticed that there is a problem to expect > IDENT_NAME_GIVEN to be set on Windows for some reason, and resulted > in a reroll of the function in that shape. > > I didn't find anything in the list archive, though. So I am > stumped. The only thing I can think of is that on Unix we can guess name from GECOS, which could be considered sufficiently your name, while on Windows we probably do not get anything useful there. -- 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: Why is ident_is_sufficient different on Windows?
Junio C Hamano writes: > Junio C Hamano writes: > >> I suspect somebody from the Windows camp saw a patch I posted >> without the ifdef, noticed that there is a problem to expect >> IDENT_NAME_GIVEN to be set on Windows for some reason, and resulted >> in a reroll of the function in that shape. >> >> I didn't find anything in the list archive, though. So I am >> stumped. > > The only thing I can think of is that on Unix we can guess name from > GECOS, which could be considered sufficiently your name, while on > Windows we probably do not get anything useful there. http://thread.gmane.org/gmane.comp.version-control.git/137312/focus=137345 These days, we encourage setting user.name explicitly even on a system on which it is likely that we will see a good GECOS value, so removing the ifdef and always check with ALL may not hurt anybody. I dunno. -- 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 0/4] Make git-send-email git-credential
Michal Nazarewicz writes: > On second thought, give me a moment, ;) I've just discovered a bug > preventing git-send-email from mailing a patchset. I somehow found this highly amusing. I wish all the bugs are like that: if your series is buggy, some parts of the system prevents you from sending it to the list. ;-) -- 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 v4] submodule: add 'deinit' command
With "git submodule init" the user is able to tell git he cares about one or more submodules and wants to have it populated on the next call to "git submodule update". But currently there is no easy way he could tell git he does not care about a submodule anymore and wants to get rid of his local work tree (except he knows a lot about submodule internals and removes the "submodule.$name.url" setting from .git/config together with the work tree himself). Help those users by providing a 'deinit' command. This removes the whole submodule. section from .git/config either for the given submodule(s) or for all those which have been initialized if '.' is given. Fail if the current work tree contains modifications unless forced. Complain when for a submodule given on the command line the url setting can't be found in .git/config, but nonetheless don't fail. Add tests and link the man pages of "git submodule deinit" and "git rm" to assist the user in deciding whether removing or unregistering the submodule is the right thing to do for him. Signed-off-by: Jens Lehmann --- Changes since v3: - Add deinit to the --force documentation of "git submodule" - Never remove submodules containing a .git dir, even when forced - Diagnostic output when "rm -rf" or "mkdir" fails - More test cases Documentation/git-rm.txt| 4 ++ Documentation/git-submodule.txt | 18 +++- git-submodule.sh| 78 ++- t/t7400-submodule-basic.sh | 100 4 files changed, 198 insertions(+), 2 deletions(-) diff --git a/Documentation/git-rm.txt b/Documentation/git-rm.txt index 92bac27..1d876c2 100644 --- a/Documentation/git-rm.txt +++ b/Documentation/git-rm.txt @@ -149,6 +149,10 @@ files that aren't ignored are present in the submodules work tree. Ignored files are deemed expendable and won't stop a submodule's work tree from being removed. +If you only want to remove the local checkout of a submodule from your +work tree without committing the removal, +use linkgit:git-submodule[1] `deinit` instead. + EXAMPLES `git rm Documentation/\*.txt`:: diff --git a/Documentation/git-submodule.txt b/Documentation/git-submodule.txt index a0c9df8..bc06159 100644 --- a/Documentation/git-submodule.txt +++ b/Documentation/git-submodule.txt @@ -13,6 +13,7 @@ SYNOPSIS [--reference ] [--] [] 'git submodule' [--quiet] status [--cached] [--recursive] [--] [...] 'git submodule' [--quiet] init [--] [...] +'git submodule' [--quiet] deinit [-f|--force] [--] ... 'git submodule' [--quiet] update [--init] [--remote] [-N|--no-fetch] [--rebase] [--reference ] [--merge] [--recursive] [--] [...] 'git submodule' [--quiet] summary [--cached|--files] [(-n|--summary-limit) ] @@ -134,6 +135,19 @@ init:: the explicit 'init' step if you do not intend to customize any submodule locations. +deinit:: + Unregister the given submodules, i.e. remove the whole + `submodule.$name` section from .git/config together with their work + tree. Further calls to `git submodule update`, `git submodule foreach` + and `git submodule sync` will skip any unregistered submodules until + they are initialized again, so use this command if you don't want to + have a local checkout of the submodule in your work tree anymore. If + you really want to remove a submodule from the repository and commit + that use linkgit:git-rm[1] instead. ++ +If `--force` is specified, the submodule's work tree will be removed even if +it contains local modifications. + update:: Update the registered submodules, i.e. clone missing submodules and checkout the commit specified in the index of the containing repository. @@ -213,8 +227,10 @@ OPTIONS -f:: --force:: - This option is only valid for add and update commands. + This option is only valid for add, deinit and update commands. When running add, allow adding an otherwise ignored submodule path. + When running deinit the submodule work trees will be removed even if + they contain local changes. When running update, throw away local changes in submodules when switching to a different commit; and always run a checkout operation in the submodule, even if the commit listed in the index of the diff --git a/git-submodule.sh b/git-submodule.sh index 004c034..f1b552f 100755 --- a/git-submodule.sh +++ b/git-submodule.sh @@ -8,6 +8,7 @@ dashless=$(basename "$0" | sed -e 's/-/ /') USAGE="[--quiet] add [-b ] [-f|--force] [--name ] [--reference ] [--] [] or: $dashless [--quiet] status [--cached] [--recursive] [--] [...] or: $dashless [--quiet] init [--] [...] + or: $dashless [--quiet] deinit [-f|--force] [--] ... or: $dashless [--quiet] update [--init] [--remote] [-N|--no-fetch] [-f|--force] [--rebase] [--reference ] [--merge] [--recursive] [--] [...] or: $dashless [--quiet] summary
Re: will git provide `submodule remove` option ?
Am 05.02.2013 11:32, schrieb Lingcha X: > As we all know, git provides `submodule add , init, update, sync, sumary, > foreach, status`, but where is `submodule remove`? > > will > I not delete one of them sometime in the future? Although most people > will not use submodule or one who uses it can remove submodule by hand, > providing complete options may be a good idea. Is assume either "git rm " (available since 1.8.1) or the upcoming "git submodule deinit" (currently in pu) will do what you want? -- 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 v3 0/8] Hiding refs
On 02/06/2013 08:17 PM, Junio C Hamano wrote: > Duy Nguyen writes: > >> On Tue, Feb 5, 2013 at 5:29 PM, Michael Haggerty >> wrote: >>> Hiderefs creates a "dark" corner of a remote git repo that can hold >>> arbitrary content that is impossible for anybody to discover but >>> nevertheless possible for anybody to download (if they know the name of >>> a hidden reference). In earlier versions of the patch series I believe >>> that it was possible to push to a hidden reference hierarchy, which made >>> it possible to upload dark content. The new version appears (from the >>> code) to prohibit adding references in a hidden hierarchy, which would >>> close the main loophole that I was worried about. But the documentation >>> and the unit tests only explicitly say that updates and deletes are >>> prohibited; nothing is said about adding references (unless "update" is >>> understood to include "add"). I think the true behavior should be >>> clarified and tested. >>> >>> I was worried that somehow this "dark" content could be used for >>> malicious purposes; for example, pushing compromised code then >>> convincing somebody to download it by SHA1 with the implicit argument >>> "it's safe since it comes directly from the project's official >>> repository". If it is indeed impossible to populate the dark namespace >>> remotely then I can't think of a way to exploit it. >> >> Or you can think hiderefs is the first step to addressing the >> initial ref advertisment problem. The series says hidden refs are >> to be fetched out of band, but that's not the only way. > > Let me help unconfuse this thread. > > I think the series as 8-patch series was poorly presented, and > separating it into two will help understanding what they are about. > > The first three: > > upload-pack: share more code > upload-pack: simplify request validation > upload/receive-pack: allow hiding ref hierarchies > > is _the_ topic of the series. As far as I am concerned (I am not > speaking for Gerrit users, but am speaking as the Git maintainer), > the topic is solely about uncluttering. There may be refs that the > server end may need to keep for its operation, but that remote users > have _no_ business knowing about. Allowing the server to keep these > refs in the repository, while not showing these refs over the wire, > is the problem the series solves. > > In other words, it is not about "these are *usually* not wanted by > clients, so do not show them by default". It is about "these are > not to be shown, ever". > > OK? Yes, the first three patches sound much more reasonable if this is the goal. Do you know of users who want the feature defined by the first three patches, or is it only a stepping stone towards an actually useful feature? (I ask because I have trouble imagining a real-world scenario where these alone would be useful.) > Now, there may be some refs that are not *usually* wanted by clients > but there may be cases where clients want to > > (1) learn about them via the same protocol; and/or > (2) fetch them over the protocol. > > If you want to solve both of these two issues generally, the > solution has to involve a separate protocol from the today's > protocol. It would go like this: [... omitted clear explanation of how delayed advertisement could be implemented via a new protocol ...] > But in the meantime, if there is a niche use case where a solution > to only the second problem is sufficient (and Gerrit and GitHub pull > requests could both be such use cases), the remainder of the series > can help, without waiting the solution to solve "usually not wanted > but may need to be learned" problem. That is the latter 4 patches > (the very last one is a demonstration to illustrate why allowing a > push to hidden ref hierarchy would not and should not work, and is > not for application): Given that some people *do* want to fetch all pull requests, is this a feature that any hosting service would really turn on? True, the majority of users would be spared clutter, but at the cost of completely preventing other users from fetching all pull requests, mirroring the repository, etc. In other words, I wonder whether your two incremental steps are useful at all, in the real world, without yet-to-be-implemented future changes. If not, then it doesn't make sense to merge them without at least imagining the final goal and gaining confidence that they are not false starts. I think that a more useful interim solution would be to make it easy to have two URLs accessing a single git repository, with different levels of reference visibility applied to each. This is something that providers could turn on without sacrificing any existing functionality. And it would solve all three problems: clutter, bandwidth, and provenance. Your first three patches would allow two-tier access to be implemented, for example by setting GIT_CONFIG or GIT_CONFIG_PARAMETERS or command-line parameters differently for the
Re: [PATCH] git-send-email: add ~/.authinfo parsing
On Wed, Feb 06, 2013 at 10:58:13AM -0500, Ted Zlatanov wrote: > MM> I don't know about the netrc credential helper, but I guess that's > MM> another layer. The git-remote-mediawiki code is the code to call the > MM> credential C API, that in turn may (or may not) call a credential > MM> helper. > > Yup. But what you call "read" and "write" are, to the credential > helper, "write" and "read" but it's the same protocol :) So maybe the > names should be changed to reflect that, e.g. "query" and "response." Is that true? As a user of the credential system, git-remote-mediawiki would want to "write" to git-credential, then "read" the response. As a helper, git-credential-netrc would want to "read" the query then "write" the response. The order is different, but the operations should be the same in both cases. The big difference is that mediawiki would want an additional function to open a pipe to "git credential" and operate on that, whereas the helper will be reading/writing stdio. -Peff -- 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] connect.c: Tell *PLink to always use ssh protocol
Default values for *plink can be set using PuTTY. If a user makes telnet the default in PuTTY this breaks ssh clones in git. Since git clones of the type user@host:path use ssh, tell *plink to use ssh and override PuTTY defaults for the protocol to use. Signed-off-by: Sven Strickroth --- connect.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/connect.c b/connect.c index 49e56ba..d337b6f 100644 --- a/connect.c +++ b/connect.c @@ -625,6 +625,8 @@ struct child_process *git_connect(int fd[2], const char *url_orig, if (!ssh) ssh = "ssh"; *arg++ = ssh; + if (putty) + *arg++ = "-ssh"; if (putty && !strcasestr(ssh, "tortoiseplink")) *arg++ = "-batch"; if (port) { -- Best regards, Sven Strickroth PGP key id F5A9D4C4 @ any key-server -- 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: [RFC/PATCH 1/4] show: obey --textconv for blobs
On Wed, Feb 06, 2013 at 04:08:50PM +0100, Michael J Gruber wrote: > diff --git a/builtin/log.c b/builtin/log.c > index 8f0b2e8..f83870d 100644 > --- a/builtin/log.c > +++ b/builtin/log.c > @@ -402,10 +402,28 @@ static void show_tagger(char *buf, int len, struct > rev_info *rev) > strbuf_release(&out); > } > > -static int show_blob_object(const unsigned char *sha1, struct rev_info *rev) > +static int show_blob_object(const unsigned char *sha1, struct rev_info *rev, > const char *obj_name) Should this maybe just take the whole object_array_entry as a cleanup? > { > + unsigned char sha1c[20]; > + struct object_context obj_context; > + char *buf; > + unsigned long size; > + > fflush(stdout); > - return stream_blob_to_fd(1, sha1, NULL, 0); > + if (!DIFF_OPT_TST(&rev->diffopt, ALLOW_TEXTCONV)) > + return stream_blob_to_fd(1, sha1, NULL, 0); > + > + if (get_sha1_with_context(obj_name, 0, sha1c, &obj_context)) > + die("Not a valid object name %s", obj_name); It seems a little hacky that we have to look up the sha1 again. What should happen in the off chance that "hashcmp(sha1, sha1c) != 0" due to a race with a simultaneous update of a ref? Would it be better if object_array_entry replaced its "mode" member with an object_context? The only downside I see is that we might waste a significant amount of memory (each context has a PATH_MAX buffer in it). -Peff -- 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 v3 0/8] Hiding refs
On 02/06/2013 08:55 PM, Jonathan Nieder wrote: > Michael Haggerty wrote: > [...] >> But now every time I do a "gitk --all" or "git log --decorate", the >> output is cluttered with all of his references (most of which are just >> old versions of references from the upstream repository that we both >> use). I would like to be able to hide his references most of the time >> but turn them back on when I need them. >> >> Scenario 5: Our upstream repository has gazillions of release tags under >> "refs/tags/releases/...", sometimes including customer-specific >> releases. In my daily life these are just clutter. > > For both of these use cases, putting the refs somewhere other than > refs/heads, refs/tags, and refs/remotes should be enough to avoid > clutter. Thanks, yes, for release tags in particular your suggestion might be workable. But I also like the idea of being able to turn subsets of references on and off easily, and have the choice persist until I change it. > [...] >> * Some small improvements (e.g. allowing *multiple* views to be >> defined) would provide much more benefit for about the same effort, >> and would be a better base for building other features in the future >> (e.g., local views). > > Would advertising GIT_CONFIG_PARAMETERS and giving examples for server > admins to set it in inetd et al to provide different kinds of access > to a same repository through different URLs work? > >> Thanks for listening. >> Michael >> >> [1] Theoretically one could support multiple views of a single >> repository by using something like "GIT_CONFIG=view_1_config git >> upload-pack ..." or "git -c transfer.hiderefs=... git upload-pack ...", >> but this would be awkward. > > Ah, I missed this comment before. What's awkward about that? I > think it's a clean way to make many aspects of how a repository is > presented (including hook actions) configurable. Awkwardness using GIT_CONFIG: the admin would have to maintain two separate config files with mostly overlapping content. Awkwardness using GIT_CONFIG_PARAMETERS or "-c transfer.hiderefs=...": the hiderefs configuration would have to be maintained in some Apache config or inetd or ... (or multiple places!) rather than in the repository's config file, where it belongs. Additional awkwardness using "-c transfer.hiderefs=...": AFAIK there is no way to turn *off* a configuration variable via a command-line option. It's all doable, but I find it awkward. 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: [RFC/PATCH 1/4] show: obey --textconv for blobs
On Wed, Feb 06, 2013 at 08:53:52AM -0800, Junio C Hamano wrote: > Michael J Gruber writes: > > > Currently, "diff" and "cat-file" for blobs obey "--textconv" options > > (with the former defaulting to "--textconv" and the latter to > > "--no-textconv") whereas "show" does not obey this option, even though > > it takes diff options. > > > > Make "show" on blobs behave like "diff", i.e. obey "--textconv" by > > default and "--no-textconv" when given. > > What does "log -p" do currently, and what should it do? Does/should > it also use --textconv? > > The --textconv is a natural extension of what --ext-diff provides us, > so I think it should trigger the same way as how --ext-diff triggers. > > We apply "--ext-diff" for "diff" by default but not for "log -p" and > "show"; I suspect this may have been for a good reason but I do not > recall the discussion that led to the current behaviour offhand. I think Michael's commit message explains the situation badly. --textconv is already on for "git show" (and for "git log") by default. Diffs already use it. This is more about the fact that when showing a single blob, we do not bother to remember the context of the sha1 lookup, including its pathname. Therefore we were not previously able to apply any .gitattributes to the output. So this patch really does two things: 1. Pass the information along to show_blob_object so that it can look up .gitattributes. 2. Apply the textconv attribute (if ALLOW_TEXTCONV is on, of course). And stating it that way makes it clear that there may be other missing steps (3 and up) to apply other gitattributes. For example, should "git show $blob" respect crlf attributes? Smudge filters? -Peff -- 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 v3 0/8] Hiding refs
Michael Haggerty writes: > On 02/06/2013 08:17 PM, Junio C Hamano wrote: > ... > > Yes, the first three patches sound much more reasonable if this is the > goal. > ... > I think that a more useful interim solution would be to make it easy to > have two URLs accessing a single git repository, with different levels > of reference visibility applied to each. I think you said "more reasonable" without understanding what you are saying is "more reasonable", then. The mechanism is about server side wanting to use refs to protect its own metadata from gc without having to expose them; there is no "different levels". > ... > GIT_CONFIG=config-with-hidden-refs git upload-pack ... > or > git -c transfer.hiderefs=refs/pull upload-pack ... > > But this is a bit awkward ... It is awkward to use hammer to drive screws in wood, too. You want to use a screwdriver. The first three patches are to drive a nail with hammer, OK? Screws you keep bringing up is to be handled by delayed ref advertisement. -- 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: [RFC/PATCH 2/4] cat-file: do not die on --textconv without textconv filters
On Wed, Feb 06, 2013 at 04:08:51PM +0100, Michael J Gruber wrote: > When a command is supposed to use textconv filters (by default or with > "--textconv") and none are configured then the blob is output without > conversion; the only exception to this rule is "cat-file --textconv". > > Make it behave like the rest of textconv aware commands. Makes sense. > - if (!textconv_object(obj_context.path, obj_context.mode, sha1, > 1, &buf, &size)) > - die("git cat-file --textconv: unable to run textconv on > %s", > - obj_name); > - break; > + if (textconv_object(obj_context.path, obj_context.mode, sha1, > 1, &buf, &size)) > + break; The implication here is that textconv_object should be handling its own errors and dying, and the return is always "yes, I converted" or "no, I did not". Which I think is the case. > + > + /* otherwise expect a blob */ > + exp_type = "blob"; > > case 0: > if (type_from_string(exp_type) == OBJ_BLOB) { I wondered at first why we needed to set exp_type here; shouldn't we already be expecting a blob if we are doing textconv? But then I see this is really about the fall-through in the switch (which we might want an explicit comment for). Which made me wonder: what happens with: git cat-file --textconv HEAD It looks like we die just before textconv-ing, because we have no obj_context.path. But that is also unlike all of the other --textconv switches, which mean "turn on textconv if you are showing a blob that supports it" and not "the specific operation is --textconv, apply it to this blob". I don't know if that is worth changing or not. -Peff -- 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: [RFC/PATCH 3/4] grep: allow to use textconv filters
On Wed, Feb 06, 2013 at 04:08:52PM +0100, Michael J Gruber wrote: > From: Jeff King > > Recently and not so recently, we made sure that log/grep type operations > use textconv filters when a userfacing diff would do the same: > > ef90ab6 (pickaxe: use textconv for -S counting, 2012-10-28) > b1c2f57 (diff_grep: use textconv buffers for add/deleted files, 2012-10-28) > 0508fe5 (combine-diff: respect textconv attributes, 2011-05-23) > > "git grep" currently does not use textconv filters at all, that is > neither for displaying the match and context nor for the actual grepping. > > Introduce an option "--textconv" which makes git grep use any configured > textconv filters for grepping and output purposes. It is off by default. > > Signed-off-by: Michael J Gruber Signed-off-by: Jeff King I'd really love to see the refactoring I talked about in my earlier message. But as I'm not willing to devote the time to do it right now, and I do not think this patch has any particular bugs, I think it is OK as it gets the job done, and does not make the later refactoring any harder. The one ugliness that still remains is: > + if (opt->allow_textconv) { > + grep_source_load_driver(gs); > + /* > + * We might set up the shared textconv cache data here, which > + * is not thread-safe. > + */ > + grep_attr_lock(); > + textconv = userdiff_get_textconv(gs->driver); > + grep_attr_unlock(); > + } We lock/unlock the grep_attr_lock twice here: once in grep_source_load_driver, and then immediately again to call userdiff_get_textconv. I don't know if it is worth doing the two under the same lock or not (I guess it should not increase lock contention, since we do the same amount of work, so it is really just the extra lock instructions). -Peff -- 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: [RFC/PATCH 2/4] cat-file: do not die on --textconv without textconv filters
Jeff King writes: > Which made me wonder: what happens with: > > git cat-file --textconv HEAD > > It looks like we die just before textconv-ing, because we have no > obj_context.path. But that is also unlike all of the other --textconv > switches, which mean "turn on textconv if you are showing a blob that > supports it" and not "the specific operation is --textconv, apply it to > this blob". I don't know if that is worth changing or not. OK, so in that sense, "cat-file --textconv HEAD" (or HEAD:) should die with "Hey, that is not a blob", in other words, Michael's patch does what we want without further tweaks, right? By the way are we sure textconv_object() barfs and dies if fed a non blob? Otherwise the above does not hold, and the semantics become "turn on textconv if the object you are showing supports it, otherwise it has to be a blob.", I think. -- 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 v3 0/8] Hiding refs
On Wed, Feb 6, 2013 at 8:17 PM, Junio C Hamano wrote: Maybe this should be split up into a different thread, but: > The upload-pack-2 service sits on a port different from today's > [...]. I think there's a simpler way to do this, which is that: * New clients supporting v2 of the protocol send some piece of data that would break old servers. * If that fails the new client goes "oh jeeze, I guess it's an old server", and try again with the old protocol. * The client then saves a date (or the version the server gave us) indicating that it tried the new protocol on that remote, tries again sometime later. We already covered in previous discussions how this would be simpler with the HTTP protocol, since you could just send an extra header inviting the server to speak the new protocol. But for the other transports we can just try the new protocol and retry with the old one as a fallback if it doesn't work. That'll allow us to gracefully migrate without needing to change the git:// port. Besides, I think the vast majority of users are using Git via http:// or ssh://, where we can't just change the port, but even so making people change the port when we could handle this more gracefully would be a big PITA. Adding new firewall holes is often a big bureaucratic nightmare in some organizations. -- 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: [RFC/PATCH 4/4] grep: obey --textconv for the case rev:path
On Wed, Feb 06, 2013 at 04:08:53PM +0100, Michael J Gruber wrote: > - add_object_array(object, arg, &list); > + add_object_array_with_context(object, arg, &list, > xmemdupz(&oc, sizeof(struct object_context))); If we go this route, this new _with_context variant should be used in patch 1, too. > @@ -265,9 +260,28 @@ void add_object_array_with_mode(struct object *obj, > const char *name, struct obj > objects[nr].item = obj; > objects[nr].name = name; > objects[nr].mode = mode; > + objects[nr].context = context; > array->nr = ++nr; > } This seems a little gross. Who is responsible for allocating the context? Who frees it? It looks like we duplicate it in cmd_grep. Which I think is OK, but it means all of this context infrastructure in object.[ch] is just bolted-on junk waiting for somebody to use it wrong or get confused. It does not get set, for example, by the regular setup_revisions code path. It would be nice if we could just always have the context available, then setup_revisions could set it up by default (and replace the "mode" parameter entirely). But we'd need to do something to avoid the PATH_MAX-sized buffer for each entry, as some code paths may have a large number of pending objects. -Peff -- 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: [RFC/PATCH 2/4] cat-file: do not die on --textconv without textconv filters
On Wed, Feb 06, 2013 at 02:23:36PM -0800, Junio C Hamano wrote: > Jeff King writes: > > > Which made me wonder: what happens with: > > > > git cat-file --textconv HEAD > > > > It looks like we die just before textconv-ing, because we have no > > obj_context.path. But that is also unlike all of the other --textconv > > switches, which mean "turn on textconv if you are showing a blob that > > supports it" and not "the specific operation is --textconv, apply it to > > this blob". I don't know if that is worth changing or not. > > OK, so in that sense, "cat-file --textconv HEAD" (or HEAD:) should > die with "Hey, that is not a blob", in other words, Michael's patch > does what we want without further tweaks, right? Right, it will die because we do not find a path in the object_context. For the same reason that "cat-file --textconv $sha1" would die. A more interesting case is "cat-file --textconv HEAD:Documentation", which does have a path, but not a blob. And I think that speaks to your point that we want to fall-through to the pretty-print case, not the blob case. > By the way are we sure textconv_object() barfs and dies if fed a non > blob? Otherwise the above does not hold, and the semantics become > "turn on textconv if the object you are showing supports it, > otherwise it has to be a blob.", I think. I'm not sure. The sha1 would get passed all the way down to fill_textconv. I think because sha1_valid is set, it will not try to reuse the working tree file, so we will end up in diff_populate_filespec, and we could actually textconv the tree object itself. So yeah, I think we want a check that makes sure we are working with a blob before we even call that function, and "--textconv" should just be "you must feed me a blob of the form $treeish:$path". In practice nobody wants to do anything else anyway, so let's keep the code paths simple; we can always loosen it later if there is a good reason to do so. -Peff -- 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] Verify Content-Type from smart HTTP servers
On Wed, Feb 06, 2013 at 07:56:08AM -0800, Junio C Hamano wrote: > Jeff King writes: > > > Is it worth having a strbuf_set* family of functions to match the > > strbuf_add*? We seem to have these sorts of errors with strbuf from time > > to time, and I wonder if that would make it easier (and more readable) > > to do the right thing. > > Possibly. > > The callsite below may be a poor example, though; you would need the > _reset() even if you change the _addstr() we can see in the context > to _setstr() to make sure later strbuf_*(type) will start from a > clean slate when !t anyway, no? Ah, true. Let's not worry about it, then. -Peff -- 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 v3 0/8] Hiding refs
On Wed, Feb 06, 2013 at 11:17:06AM -0800, Junio C Hamano wrote: > Let me help unconfuse this thread. > > I think the series as 8-patch series was poorly presented, and > separating it into two will help understanding what they are about. > > The first three: > > upload-pack: share more code > upload-pack: simplify request validation > upload/receive-pack: allow hiding ref hierarchies > > is _the_ topic of the series. As far as I am concerned (I am not > speaking for Gerrit users, but am speaking as the Git maintainer), > the topic is solely about uncluttering. There may be refs that the > server end may need to keep for its operation, but that remote users > have _no_ business knowing about. Allowing the server to keep these > refs in the repository, while not showing these refs over the wire, > is the problem the series solves. > > In other words, it is not about "these are *usually* not wanted by > clients, so do not show them by default". It is about "these are > not to be shown, ever". > > OK? Right. I am not opposed to this series, as it does have a use-case. And if it helps Gerrit folks or other users unclutter, great. The fact that I could throw away the custom receive.hiderefs patch we use at GitHub is a bonus. If people want fancier things, they can do them separately. _But_. As a potential user of the feature (to hide refs/pull/*), I do not think it is sufficiently flexible for me to use transfer.hiderefs (or uploadpack.hiderefs). We use "fetch" internally to migrate objects between forks and our alternates repos. And in that case, we really do want to see all refs. In other words, all fetches are not the same: we would want upload-pack to understand the difference between a client fetch and an internal administrative fetch. But this feature does not provide that lee-way. Even if you tried: git fetch -u 'git -c uploadpack.hiderefs= upload-pack' the list nature of the config variable means you cannot reset it. This isn't a show-stopper for the series; it may just mean that it is not a good fit for GitHub's use case, but others (like Gerrit) may benefit. But since refs/pull is used as an example of where this could be applied, I wanted to point out that it does not achieve that goal. -Peff -- 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 2/4] Git.pm: Allow pipes to be closed prior to calling command_close_bidi_pipe
On Wed, Feb 06, 2013 at 09:47:04PM +0100, Michal Nazarewicz wrote: > From: Michal Nazarewicz > > The command_close_bidi_pipe() function will insist on closing both > input and output pipes returned by command_bidi_pipe(). With this > change it is possible to close one of the pipes in advance and > pass undef as an argument. > > This allows for something like: > > my ($pid, $in, $out, $ctx) = command_bidi_pipe(...); > print $out "write data"; > close $out; > # ... do stuff with $in > command_close_bidi_pipe($pid, $in, undef, $ctx); Should this part go into the documentation for command_close_bidi_pipe in Git.pm? -Peff -- 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] git-send-email: add ~/.authinfo parsing
On Wed, 6 Feb 2013 16:57:24 -0500 Jeff King wrote: JK> On Wed, Feb 06, 2013 at 10:58:13AM -0500, Ted Zlatanov wrote: MM> I don't know about the netrc credential helper, but I guess that's MM> another layer. The git-remote-mediawiki code is the code to call the MM> credential C API, that in turn may (or may not) call a credential MM> helper. >> >> Yup. But what you call "read" and "write" are, to the credential >> helper, "write" and "read" but it's the same protocol :) So maybe the >> names should be changed to reflect that, e.g. "query" and "response." JK> Is that true? As a user of the credential system, git-remote-mediawiki JK> would want to "write" to git-credential, then "read" the response. As a JK> helper, git-credential-netrc would want to "read" the query then JK> "write" the response. The order is different, but the operations should JK> be the same in both cases. Logically they are different steps (query and response), even though the data protocol is the same. But it's really not a big deal, I know what it means either way. JK> The big difference is that mediawiki would want an additional function JK> to open a pipe to "git credential" and operate on that, whereas the JK> helper will be reading/writing stdio. Yup. Ted -- 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 3/4] Git.pm: Add interface for git credential command.
On Wed, Feb 06, 2013 at 09:47:05PM +0100, Michal Nazarewicz wrote: > +sub _credential_read { > + my %credential; > + my ($reader, $op) = (@_); > + while (<$reader>) { > + chomp; > + my ($key, $value) = /([^=]*)=(.*)/; Empty keys are not valid. Can we make this: /^([^=]+)=(.*)/ to fail the regex? Otherwise, I think this check: > + if (not defined $key) { > + throw Error::Simple("unable to parse git credential $op > response:\n$_\n"); > + } would not pass because $key would be the empty string. > +sub _credential_write { > + my ($credential, $writer) = @_; > + > + for my $key (sort { > + # url overwrites other fields, so it must come first > + return -1 if $a eq 'url'; > + return 1 if $b eq 'url'; > + return $a cmp $b; > + } keys %$credential) { > + if (defined $credential->{$key} && length $credential->{$key}) { > + print $writer $key, '=', $credential->{$key}, "\n"; > + } > + } There are a few disallowed characters, like "\n" in key or value, and "=" in a key. They should never happen unless the caller is buggy, but should we check and catch them here? > +In the second form, C needs to be a reference to a subroutine. > +The function will execute C to fill provided > +credential hash, than call C with C as the sole > +argument, and finally depending on C's return value execute > +C (if return value yields true) or C +credential reject> (otherwise). The return value is the same as what > +C returned. With this form, the usage might look as follows: This is a nice touch. It makes the normal calling code a lot simpler. -Peff -- 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 4/4] git-send-email: Use git credential to obtain password.
On Wed, Feb 06, 2013 at 09:47:06PM +0100, Michal Nazarewicz wrote: > From: Michal Nazarewicz > > If smtp_user is provided but smtp_pass is not, instead of prompting > for password, make git-send-email use git credential command > instead. > > Signed-off-by: Michal Nazarewicz > --- > Documentation/git-send-email.txt | 4 +-- > git-send-email.perl | 59 > +++- > 2 files changed, 36 insertions(+), 27 deletions(-) Nice. I don't see anything obviously wrong with the code, but I didn't try it myself. I wonder how hard it would be to have some tests in t9001. It looks like we don't test the smtp code paths at all, since we would have to implement a fake smtp server. Which probably means the answer is is "pretty hard", unless there is an easy-to-use CPAN smtp server module we can plug in. -Peff -- 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] connect.c: Tell *PLink to always use ssh protocol
On Wed, Feb 06, 2013 at 10:58:49PM +0100, Sven Strickroth wrote: > Default values for *plink can be set using PuTTY. If a user makes > telnet the default in PuTTY this breaks ssh clones in git. > > Since git clones of the type user@host:path use ssh, tell *plink > to use ssh and override PuTTY defaults for the protocol to use. > > Signed-off-by: Sven Strickroth Makes sense to me, though I'd expect to see this cc'd to the msysgit list (which I'm doing on this response) for comment from people who might be more familiar with the area. Quoted patch follows. -Peff > --- > connect.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/connect.c b/connect.c > index 49e56ba..d337b6f 100644 > --- a/connect.c > +++ b/connect.c > @@ -625,6 +625,8 @@ struct child_process *git_connect(int fd[2], const char > *url_orig, > if (!ssh) ssh = "ssh"; > > *arg++ = ssh; > + if (putty) > + *arg++ = "-ssh"; > if (putty && !strcasestr(ssh, "tortoiseplink")) > *arg++ = "-batch"; > if (port) { > -- > Best regards, > Sven Strickroth > PGP key id F5A9D4C4 @ any key-server -- 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: [RFC/PATCH 1/4] show: obey --textconv for blobs
Jeff King writes: > And stating it that way makes it clear that there may be other missing > steps (3 and up) to apply other gitattributes. For example, should "git > show $blob" respect crlf attributes? Smudge filters? Yeah, I think applying these when path is availble may make sense. Are we going to teach cat-file to honor them with "--textconv" and possibly other new options? Or should the "--textconv" imply application of these other filters? I am tempted to say yes, but I haven't thought things through... -- 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