Re: [PATCH] Geolocation support
Alessandro Di Marco d...@ethzero.com writes: Junio C Hamano gits...@pobox.com writes: More importantly, adding non-essential stuff left and right will force third party Git reimplementations to pay attention to them and also will leave room for them to make mistakes when deciding what to propagate, what to drop and what to update when rewriting commits via rebase, cherry-pick, etc. ??? http://en.wikipedia.org/wiki/Security_through_obscurity Do you realize that every git I tried so far has happily accepted any crufts I sent to it via git push? And that they stored that crufts and then returned it on cloning? :-| Yes, they will all copy the original commits byte-for-byte. Otherwise they are broken. But that is not the paragraph you quoted and responded is about. What *should* happen, either in the original repository or the other repository you pushed these commits into, when you _rewrite_ such a commit? Should all the cruft headers be carried over to the rewritten commit? Should all of them be dropped? Should some be kept but some be dropped? Should some be kept under one condition but not others? How are you making sure that all Git reimplementations do the same thing to the random cruft headers? -- 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] Geolocation support
Junio C Hamano gits...@pobox.com writes:
Ævar Arnfjörð Bjarmason ava...@gmail.com writes:
We've already told clients for a long time to ignore fields they
don't know about,
Yes, but the reason that mechanism is there is not because we want to
add random cruft Git does not have to know about. It is to avoid
older Git from suddenly stop working when we really need to add new
essential things.
I see, so the problem lays in the info at hand (ie. the commit
location), apparently not enough important to be promoted to the upper
floor. Admittedly all of this is difficult to appreciate unless you are
on the move (as well as the need of a date would be probably questioned
by an highlander), therefore I refined the previous approach.
The new patch here below will allow anybody to import any crufts
into the commit header. Simply define an envvar such as:
export GIT_XT_CRUFT=foo
and it will place an extra header such as:
xt-cruft foo
in the commit object. I felt free to insert the 'xt-' prefix in order
not to clash with existing and/or future headers of the commit
object. The '' indicates that the header is not amendable; in case you
want an amendable one simply switch from '' to '@' as below:
export GIT_XT_CRUFT=@foo
More importantly, adding non-essential stuff left and right will force
third party Git reimplementations to pay attention to them and also
will leave room for them to make mistakes when deciding what to
propagate, what to drop and what to update when rewriting commits via
rebase, cherry-pick, etc.
??? http://en.wikipedia.org/wiki/Security_through_obscurity
Do you realize that every git I tried so far has happily accepted any
crufts I sent to it via git push? And that they stored that crufts and
then returned it on cloning? :-|
Feel free to try the below from your client:
0 $ git clone g...@github.com:dmrlsn/iwillmeltyourdata.git
Cloning into 'iwillmeltyourdata'...
remote: Counting objects: 3, done.
remote: Total 3 (delta 0), reused 3 (delta 0)
Receiving objects: 100% (3/3), done.
Checking connectivity... done.
0 $ cd iwillmeltyourdata/
0 $ git log --pretty=raw | more
commit b704144270528fc6022714861d149441d4102ee9
tree 6bf21934b3b186561626891e2b98f99e6da89e2f
author Alessandro Di Marco d...@ethzero.com 1423770339 +0100
committer Alessandro Di Marco d...@ethzero.com 1423770339 +0100
xt-committer-location Tokyo, Japan (35.685, 139.7514)
xt-author-location Tokyo, Japan (35.685, 139.7514)
Minor changes
^^ obtained by defining:
declare -x GIT_XT_AUTHOR_LOCATION=Tokyo, Japan (35.685, 139.7514)
declare -x GIT_XT_COMMITTER_LOCATION=@Tokyo, Japan (35.685, 139.7514)
prior committing.
I think this sentence gets it backwards. The question to ask is if it
is an arbitrary cruft that the end users are better off if they can
easily typofix in the commit message log editor, or is it essential
for Git to operate correctly and end users shouldn't be allowed to
muck with in the editor?
The expected location format is CITY, COUNTRY (LAT, LON).
I would expect that I can typofix Les Angeles to Los Angeles,
if I were using this feature.
Well, what about a format such as (LAT, LON)? Would you expect to
typofix it too? If so, why don't you put the date on the commit message
as well? Your clock could fail, after all...
Signed-off-by: Alessandro Di Marco d...@ethzero.com
---
builtin/commit.c | 108 +--
commit.c | 14
commit.h | 3 +-
3 files changed, 114 insertions(+), 11 deletions(-)
diff --git a/builtin/commit.c b/builtin/commit.c
index 7f46713..0ff4aef 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -623,6 +623,100 @@ static int author_date_is_interesting(void)
return author_message || force_date;
}
+static void exclude_free(char ***exclude) {
+ char **p = *exclude;
+ int len;
+
+ for (len = 0; p[len]; len++) {
+ free(p[len]);
+ }
+ free(p);
+ *exclude = NULL;
+}
+
+static void exclude_one(char ***exclude, char *what)
+{
+ char **p = *exclude;
+ int len;
+
+ for (len = 0; p p[len]; len++);
+ p = *exclude = xrealloc(p, (len + 2) * sizeof (char *));
+ p[len] = xmalloc(strlen(what) + 1);
+ p[len + 1] = NULL;
+ strcpy(p[len], what);
+}
+
+static char *env2extra(char *var)
+{
+ int i, l = strlen(var);
+
+ for (i = 0; i l; i++) {
+ if (var[i] = 'A' var[i] = 'Z') {
+ var[i] += 32;
+ } else if (var[i] == '_') {
+ var[i] = '-';
+ }
+ }
+ memcpy(var -= 3, xt-, 3);
+ return var;
+}
+
+static void determine_xt_vars(struct strbuf *xtvars, char ***exclude)
+{
+ extern char **environ;
+ int i;
+
+ for (i = 0; environ[i]; i++) {
+ char *p, *var, *xtvar, *val;
+ int l, amending;
+
+ if (strncmp(environ[i], GIT_XT_, 7)) {
+
Re: [PATCH] Geolocation support
Ævar Arnfjörð Bjarmason ava...@gmail.com writes: On Mon, Feb 9, 2015 at 2:24 AM, Junio C Hamano gits...@pobox.com wrote: In case I was not clear, I do not think it is likely for us to accept a patch that mucks with object header fields with this information. Have them in the log text and let UI interpret them. We've already told clients for a long time to ignore fields they don't know about, Yes, but the reason that mechanism is there is not because we want to add random cruft Git does not have to know about. It is to avoid older Git from suddenly stop working when we really need to add new essential things. This was discussed in great length on this list already. cf. http://thread.gmane.org/gmane.comp.version-control.git/138848/focus=138852 More importantly, adding non-essential stuff left and right will force third party Git reimplementations to pay attention to them and also will leave room for them to make mistakes when deciding what to propagate, what to drop and what to update when rewriting commits via rebase, cherry-pick, etc. why would we not store what's intended to be machine-readable key-value pair data in the commit object itself, I think this sentence gets it backwards. The question to ask is if it is an arbitrary cruft that the end users are better off if they can easily typofix in the commit message log editor, or is it essential for Git to operate correctly and end users shouldn't be allowed to muck with in the editor? The expected location format is CITY, COUNTRY (LAT, LON). I would expect that I can typofix Les Angeles to Los Angeles, if I were using this feature. -- 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] Geolocation support
On Mon, Feb 9, 2015 at 2:24 AM, Junio C Hamano gits...@pobox.com wrote: In case I was not clear, I do not think it is likely for us to accept a patch that mucks with object header fields with this information. Have them in the log text and let UI interpret them. We've already told clients for a long time to ignore fields they don't know about, why would we not store what's intended to be machine-readable key-value pair data in the commit object itself, as opposed to sticking it in the log message where parsing it is always going to be a bit more tricky distracting, since users will have to look at this arbitrary metadata when they do git log or git show. -- 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] Geolocation support
In case I was not clear, I do not think it is likely for us to accept
a patch that mucks with object header fields with this information.
Have them in the log text and let UI interpret them.
On Sun, Feb 8, 2015 at 4:03 PM, Alessandro Di Marco d...@ethzero.com wrote:
Junio C Hamano gits...@pobox.com writes:
I would personally find the feature cute, but
snip
Wouldn't it be sufficient to treat this in a similar way as
references to tracker entries and references to other commit objects
in the text of the commit message body are treated by gitk and
friends? Just embed the information in the log text somewhere and
teach the UI how they look like and what to do with them.
Sorry for the delay, I've been a little busy lately :-)
I revised the old patch and moved the location info into a pair of extra
headers, resp. author-location and committer-location. This way location
unaware clients do not try to interpret the extra stuff and things stay
smooth as usual. I tried to push/clone a location aware repository
to/from github and it seemed to work as expected (ie. location was
retained by github servers).
The patch is still rough; only gitk interpret the location info at the
moment (see http://tinypic.com/r/wars40/5), but log pretty printing is
on the way out.
The expected location format is CITY, COUNTRY (LAT, LON). Location is
provided by two envvars, resp. GIT_AUTHOR_LOCATION and
GIT_COMMITTER_LOCATION, that should be updated prior committing. E.g.
$ export GIT_AUTHOR_LOCATION=Paris, France (48.8667, 2.)
$ export GIT_COMMITTER_LOCATION=Paris, France (48.8667, 2.)
$ git commit -a -s -m foo
The patch honors the committer location on amending, etc. and gitk
reports it with a red circle on the map. Author's location is indicated
by a red point instead. Keep in mind that the soundness of the envvars
content is taken for grant on commit (ie. no sanity checks yet), so
stick carefully to the above format or gitk will whine.
Signed-off-by: Alessandro Di Marco d...@ethzero.com
---
builtin/commit.c | 48 -
commit.c | 10 +-
commit.h | 1 +
gitk-git/gitk| 552
+--
4 files changed, 590 insertions(+), 21 deletions(-)
diff --git a/builtin/commit.c b/builtin/commit.c
index 7d90c35..188f424 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -629,6 +629,38 @@ static int author_date_is_interesting(void)
return author_message || force_date;
}
+static int location_is_safe(char *loc)
+{
+ if (loc) {
+ /* FIXME: sanity checks here */
+ }
+
+ return !!loc;
+}
+
+static int determine_location(struct strbuf *location)
+{
+ char *loc;
+
+ if (!amend) {
+ loc = getenv(GIT_AUTHOR_LOCATION);
+ if (location_is_safe(loc)) {
+ strbuf_addstr(location, author-location );
+ strbuf_addstr(location, loc);
+ strbuf_addch(location, '\n');
+ }
+ }
+
+ loc = getenv(GIT_COMMITTER_LOCATION);
+ if (location_is_safe(loc)) {
+ strbuf_addstr(location, committer-location );
+ strbuf_addstr(location, loc);
+ strbuf_addch(location, '\n');
+ }
+
+ return 1;
+}
+
static void adjust_comment_line_char(const struct strbuf *sb)
{
char candidates[] = #;@!$%^|:;
@@ -1630,6 +1662,7 @@ int cmd_commit(int argc, const char **argv, const char
*prefix)
struct strbuf sb = STRBUF_INIT;
struct strbuf author_ident = STRBUF_INIT;
+ struct strbuf location = STRBUF_INIT;
const char *index_file, *reflog_msg;
char *nl;
unsigned char sha1[20];
@@ -1669,6 +1702,11 @@ int cmd_commit(int argc, const char **argv, const char
*prefix)
return 1;
}
+ if (!determine_location(location)) {
+ rollback_index_files();
+ return 1;
+ }
+
/* Determine parents */
reflog_msg = getenv(GIT_REFLOG_ACTION);
if (!current_head) {
@@ -1745,13 +1783,19 @@ int cmd_commit(int argc, const char **argv, const
char *prefix)
}
if (amend) {
- const char *exclude_gpgsig[2] = { gpgsig, NULL };
- extra = read_commit_extra_headers(current_head,
exclude_gpgsig);
+ const char *exclude[3] = { gpgsig, committer-location,
NULL };
+ extra = read_commit_extra_headers(current_head, exclude);
} else {
struct commit_extra_header **tail = extra;
append_merge_tag_headers(parents, tail);
}
+ if (location.len 0) {
+ extra = read_commit_extra_header_lines(location.buf,
+ location.len, NULL, extra);
+ }
+ strbuf_release(location);
+
if
Re: [PATCH] Geolocation support
Junio C Hamano gits...@pobox.com writes:
I would personally find the feature cute, but
snip
Wouldn't it be sufficient to treat this in a similar way as
references to tracker entries and references to other commit objects
in the text of the commit message body are treated by gitk and
friends? Just embed the information in the log text somewhere and
teach the UI how they look like and what to do with them.
Sorry for the delay, I've been a little busy lately :-)
I revised the old patch and moved the location info into a pair of extra
headers, resp. author-location and committer-location. This way location
unaware clients do not try to interpret the extra stuff and things stay
smooth as usual. I tried to push/clone a location aware repository
to/from github and it seemed to work as expected (ie. location was
retained by github servers).
The patch is still rough; only gitk interpret the location info at the
moment (see http://tinypic.com/r/wars40/5), but log pretty printing is
on the way out.
The expected location format is CITY, COUNTRY (LAT, LON). Location is
provided by two envvars, resp. GIT_AUTHOR_LOCATION and
GIT_COMMITTER_LOCATION, that should be updated prior committing. E.g.
$ export GIT_AUTHOR_LOCATION=Paris, France (48.8667, 2.)
$ export GIT_COMMITTER_LOCATION=Paris, France (48.8667, 2.)
$ git commit -a -s -m foo
The patch honors the committer location on amending, etc. and gitk
reports it with a red circle on the map. Author's location is indicated
by a red point instead. Keep in mind that the soundness of the envvars
content is taken for grant on commit (ie. no sanity checks yet), so
stick carefully to the above format or gitk will whine.
Signed-off-by: Alessandro Di Marco d...@ethzero.com
---
builtin/commit.c | 48 -
commit.c | 10 +-
commit.h | 1 +
gitk-git/gitk| 552 +--
4 files changed, 590 insertions(+), 21 deletions(-)
diff --git a/builtin/commit.c b/builtin/commit.c
index 7d90c35..188f424 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -629,6 +629,38 @@ static int author_date_is_interesting(void)
return author_message || force_date;
}
+static int location_is_safe(char *loc)
+{
+ if (loc) {
+ /* FIXME: sanity checks here */
+ }
+
+ return !!loc;
+}
+
+static int determine_location(struct strbuf *location)
+{
+ char *loc;
+
+ if (!amend) {
+ loc = getenv(GIT_AUTHOR_LOCATION);
+ if (location_is_safe(loc)) {
+ strbuf_addstr(location, author-location );
+ strbuf_addstr(location, loc);
+ strbuf_addch(location, '\n');
+ }
+ }
+
+ loc = getenv(GIT_COMMITTER_LOCATION);
+ if (location_is_safe(loc)) {
+ strbuf_addstr(location, committer-location );
+ strbuf_addstr(location, loc);
+ strbuf_addch(location, '\n');
+ }
+
+ return 1;
+}
+
static void adjust_comment_line_char(const struct strbuf *sb)
{
char candidates[] = #;@!$%^|:;
@@ -1630,6 +1662,7 @@ int cmd_commit(int argc, const char **argv, const char
*prefix)
struct strbuf sb = STRBUF_INIT;
struct strbuf author_ident = STRBUF_INIT;
+ struct strbuf location = STRBUF_INIT;
const char *index_file, *reflog_msg;
char *nl;
unsigned char sha1[20];
@@ -1669,6 +1702,11 @@ int cmd_commit(int argc, const char **argv, const char
*prefix)
return 1;
}
+ if (!determine_location(location)) {
+ rollback_index_files();
+ return 1;
+ }
+
/* Determine parents */
reflog_msg = getenv(GIT_REFLOG_ACTION);
if (!current_head) {
@@ -1745,13 +1783,19 @@ int cmd_commit(int argc, const char **argv, const char
*prefix)
}
if (amend) {
- const char *exclude_gpgsig[2] = { gpgsig, NULL };
- extra = read_commit_extra_headers(current_head, exclude_gpgsig);
+ const char *exclude[3] = { gpgsig, committer-location, NULL
};
+ extra = read_commit_extra_headers(current_head, exclude);
} else {
struct commit_extra_header **tail = extra;
append_merge_tag_headers(parents, tail);
}
+ if (location.len 0) {
+ extra = read_commit_extra_header_lines(location.buf,
+ location.len, NULL, extra);
+ }
+ strbuf_release(location);
+
if (commit_tree_extended(sb.buf, sb.len, active_cache_tree-sha1,
parents, sha1, author_ident.buf, sign_commit, extra)) {
rollback_index_files();
diff --git a/commit.c b/commit.c
index a8c7577..0b4e66e 100644
--- a/commit.c
+++ b/commit.c
@@ -12,8 +12,6 @@
#include prio-queue.h
#include sha1-lookup.h
-static struct commit_extra_header
Re: [PATCH] Geolocation support
Alessandro Di Marco wrote: this is a hack I made a couple of years ago in order to store my current location in git commits (I travel a lot and being able to associate a place with the commit date helps me to quickly recover what were doing at that time). Long story short, the screeenshot at http://tinypic.com/r/wars40/5 shows the new gitk interface once this patch has been integrated. Geolocation is controlled by two envvars GIT_AUTHOR_PLACE and COMMITTER_PLACE, respectively. You can set them via something like this: Obviously very interesting. Now, how do we mainline (parts of) this feature? I'll raise some questions here: 0. We already have timezone information, but this is obviously insufficient for any sensible geolocation data. 1. Does it make sense to make it an optional field in the commit object? I can see how generic optional fields in the commit object can be useful: a lot of code-review systems put the code-review ID in the commit message, and I can see how an optional field would benefit them. Will it break existing parsers (shouldn't they ignore unknown fields)? 2. How accurate should this geolocation information for it to be invariant enough? If we blindly store what a GPS gives us, the centering error is obviously a problem. What should be the resolution of the lat/long that we store? 3. Failing (2), can we put the geolocation data in the commit message, and proceed? If so, does it need to be part of git-core, or should an external client (gitk, or other clients) write/ parse the geolocation information? -- 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] Geolocation support
On Thu, May 23, 2013 at 10:45 AM, Ramkumar Ramachandra artag...@gmail.com wrote: Alessandro Di Marco wrote: this is a hack I made a couple of years ago in order to store my current location in git commits (I travel a lot and being able to associate a place with the commit date helps me to quickly recover what were doing at that time). Long story short, the screeenshot at http://tinypic.com/r/wars40/5 shows the new gitk interface once this patch has been integrated. Geolocation is controlled by two envvars GIT_AUTHOR_PLACE and COMMITTER_PLACE, respectively. You can set them via something like this: Obviously very interesting. Now, how do we mainline (parts of) this feature? I'll raise some questions here: I'm really not convinced this kind of changes should make it into Junio's tree (of course, he's the only one to decide). I really believe this is a very specific solution to a very specific problem (that is not for me to judge if the problem is real). Bloating the commit object with this kind of information doesn't feel like a good idea. I think it could be nice to provide a simple shell script to build the location, callable from a post-commit hook, to construct a geolocation note. Gitk could be programmed to read the notes to get the location, but once again, I'm not sure it should be mainlined. -- 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] Geolocation support
Antoine Pelisse apeli...@gmail.com writes: On Thu, May 23, 2013 at 10:45 AM, Ramkumar Ramachandra artag...@gmail.com wrote: Alessandro Di Marco wrote: this is a hack I made a couple of years ago in order to store my current location in git commits (I travel a lot and being able to associate a place with the commit date helps me to quickly recover what were doing at that time). Long story short, the screeenshot at http://tinypic.com/r/wars40/5 shows the new gitk interface once this patch has been integrated. Geolocation is controlled by two envvars GIT_AUTHOR_PLACE and COMMITTER_PLACE, respectively. You can set them via something like this: Obviously very interesting. Now, how do we mainline (parts of) this feature? I'll raise some questions here: I think it could be nice to provide a simple shell script to build the location, callable from a post-commit hook, to construct a geolocation note. Gitk could be programmed to read the notes to get the location, but once again, I'm not sure it should be mainlined. Well, I don't see how the file can be kept synchronized with the tree, but in case it would be also suitable for the author/committer name, email and date :-) Seriously, this is just a hack; the other nice thing coming out from this patch is what I called the Project's Patch Graph (or PPG), ie. a DAG starting from the project founder location and spreading all over the world (depending on the project of course!) IMHO it's an interesting snapshot of how the project is evolving. -- 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] Geolocation support
Antoine Pelisse apeli...@gmail.com writes: I'm really not convinced this kind of changes should make it into Junio's tree (of course, he's the only one to decide). I really believe this is a very specific solution to a very specific problem (that is not for me to judge if the problem is real). Bloating the commit object with this kind of information doesn't feel like a good idea. I think it could be nice to provide a simple shell script to build the location, callable from a post-commit hook, to construct a geolocation note. Gitk could be programmed to read the notes to get the location, but once again, I'm not sure it should be mainlined. I would personally find the feature cute, but - I think a note is an overkill for that; and - I also think that adding cruft to commit object header in a backward incompatible way, with the only possible escape-hatch being if you want to keep being compatible with other people, you can choose not to use this feature, is a slipperly slope to fragmentation we do not want to go nearby. Wouldn't it be sufficient to treat this in a similar way as references to tracker entries and references to other commit objects in the text of the commit message body are treated by gitk and friends? Just embed the information in the log text somewhere and teach the UI how they look like and what to do with them. -- 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
