Re: [PATCH 01/15] builtin/add.c: rearrange xcalloc arguments
On Wed, May 28, 2014 at 7:41 AM, Junio C Hamano wrote: > I do not think it is worth doing this change starting from maint, so > I've dropped this one and a few others that did not apply to master > and queued the remainder to 'pu'. Thank you! I'll keep this in mind when choosing what to branch off of in the future. On Wed, May 28, 2014 at 6:35 AM, Eric Sunshine wrote: > Etiquette on this list is to avoid top-posting [1]. > ... > If you do re-roll, perhaps consider simplifying the commit messages. Thank you for the tips; very much appreciated. - Brian Gesiak -- 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 01/15] builtin/add.c: rearrange xcalloc arguments
Oomph, how embarrassing. Thanks for pointing that out! Would it be better if I rerolled the patches? - Brian Gesiak On Tue, May 27, 2014 at 12:25 PM, Eric Sunshine wrote: > On Mon, May 26, 2014 at 11:33 AM, Brian Gesiak wrote: >> xcalloc takes two arguments: the number of elements and their size. >> run_add_interactive passes the arguments in reverse order, passing the >> size of a char*, followed by the number of char* to be allocated. >> Rearrgange them so they are in the correct order. > > s/Rearrgange/Rearrange/ > > Same misspelling afflicts the entire patch series. > >> Signed-off-by: Brian Gesiak >> --- >> builtin/add.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/builtin/add.c b/builtin/add.c >> index 672adc0..488acf4 100644 >> --- a/builtin/add.c >> +++ b/builtin/add.c >> @@ -248,7 +248,7 @@ int run_add_interactive(const char *revision, const char >> *patch_mode, >> int status, ac, i; >> const char **args; >> >> - args = xcalloc(sizeof(const char *), (pathspec->nr + 6)); >> + args = xcalloc((pathspec->nr + 6), sizeof(const char *)); >> ac = 0; >> args[ac++] = "add--interactive"; >> if (patch_mode) >> -- >> 2.0.0.rc1.543.gc8042da -- 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 01/15] builtin/add.c: rearrange xcalloc arguments
My apologies! I based my work off of maint, branching off of eea591. My reasoning was that Documentation/SubmittingPatches states that "a bugfix should be based on 'maint'". [1] Now that I think about it, this is probably not the kind of "bug" that statement had in mind. Should I reroll the patch based on master? - Brian Gesiak [1] https://github.com/git/git/blob/4a28f169ad29ba452e0e7bea2583914c10c58322/Documentation/SubmittingPatches#L9 On Tue, May 27, 2014 at 8:11 AM, Jeremiah Mahler wrote: > Brian, > > On Tue, May 27, 2014 at 12:33:42AM +0900, Brian Gesiak wrote: >> xcalloc takes two arguments: the number of elements and their size. >> run_add_interactive passes the arguments in reverse order, passing the >> size of a char*, followed by the number of char* to be allocated. >> Rearrgange them so they are in the correct order. >> >> Signed-off-by: Brian Gesiak >> --- >> builtin/add.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/builtin/add.c b/builtin/add.c >> index 672adc0..488acf4 100644 >> --- a/builtin/add.c >> +++ b/builtin/add.c >> @@ -248,7 +248,7 @@ int run_add_interactive(const char *revision, const char >> *patch_mode, >> int status, ac, i; >> const char **args; >> >> - args = xcalloc(sizeof(const char *), (pathspec->nr + 6)); >> + args = xcalloc((pathspec->nr + 6), sizeof(const char *)); >> ac = 0; >> args[ac++] = "add--interactive"; >> if (patch_mode) >> > > This patch doesn't apply to any of the branches I have available > (master, pu, next). And there is no line containing "pathspec->nr + 6" > anywhere in my builtin/add.c. Which branch is your work based off? > > -- > Jeremiah Mahler > jmmah...@gmail.com > http://github.com/jmahler -- 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 07/15] hash.c: rearrange xcalloc arguments
xcalloc takes two arguments: the number of elements and their size. grow_hash_table passes the arguments in reverse order, passing the size of a hash table entry, followed by the number of entries. Rearrgange them so they are in the correct order. Signed-off-by: Brian Gesiak --- hash.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/hash.c b/hash.c index 749ecfe..2067be9 100644 --- a/hash.c +++ b/hash.c @@ -53,7 +53,7 @@ static void grow_hash_table(struct hash_table *table) struct hash_table_entry *old_array = table->array, *new_array; new_size = alloc_nr(old_size); - new_array = xcalloc(sizeof(struct hash_table_entry), new_size); + new_array = xcalloc(new_size, sizeof(struct hash_table_entry)); table->size = new_size; table->array = new_array; table->nr = 0; -- 2.0.0.rc1.543.gc8042da -- 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 14/15] remote.c: rearrange xcalloc arguments
xcalloc takes two arguments: the number of elements and their size. parse_refspec_internal passes the arguments in reverse order, passing the size of a refspec, followed by the number to allocate. Rearrgange them so they are in the correct order. Signed-off-by: Brian Gesiak --- remote.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/remote.c b/remote.c index ebed40d..df3267b 100644 --- a/remote.c +++ b/remote.c @@ -523,7 +523,7 @@ static void free_refspecs(struct refspec *refspec, int nr_refspec) static struct refspec *parse_refspec_internal(int nr_refspec, const char **refspec, int fetch, int verify) { int i; - struct refspec *rs = xcalloc(sizeof(*rs), nr_refspec); + struct refspec *rs = xcalloc(nr_refspec, sizeof(*rs)); for (i = 0; i < nr_refspec; i++) { size_t llen; -- 2.0.0.rc1.543.gc8042da -- 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 11/15] notes.c: rearrange xcalloc arguments
xcalloc takes two arguments: the number of elements and their size. notes.c includes several calls to xcalloc that pass the arguments in reverse order. Rearrgange them so they are in the correct order. Signed-off-by: Brian Gesiak --- notes.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/notes.c b/notes.c index 5f07c0b..5fe691d 100644 --- a/notes.c +++ b/notes.c @@ -303,7 +303,7 @@ static int note_tree_insert(struct notes_tree *t, struct int_node *tree, free(entry); return 0; } - new_node = (struct int_node *) xcalloc(sizeof(struct int_node), 1); + new_node = (struct int_node *) xcalloc(1, sizeof(struct int_node)); ret = note_tree_insert(t, new_node, n + 1, l, GET_PTR_TYPE(*p), combine_notes); if (ret) @@ -443,7 +443,7 @@ static void load_subtree(struct notes_tree *t, struct leaf_node *subtree, if (len <= 20) { type = PTR_TYPE_NOTE; l = (struct leaf_node *) - xcalloc(sizeof(struct leaf_node), 1); + xcalloc(1, sizeof(struct leaf_node)); hashcpy(l->key_sha1, object_sha1); hashcpy(l->val_sha1, entry.sha1); if (len < 20) { @@ -1003,7 +1003,7 @@ void init_notes(struct notes_tree *t, const char *notes_ref, if (!combine_notes) combine_notes = combine_notes_concatenate; - t->root = (struct int_node *) xcalloc(sizeof(struct int_node), 1); + t->root = (struct int_node *) xcalloc(1, sizeof(struct int_node)); t->first_non_note = NULL; t->prev_non_note = NULL; t->ref = notes_ref ? xstrdup(notes_ref) : NULL; -- 2.0.0.rc1.543.gc8042da -- 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 09/15] http-push.c: rearrange xcalloc arguments
xcalloc takes two arguments: the number of elements and their size. http-push passes the arguments in reverse order, passing the size of a repo, followed by the number to allocate. Rearrgange them so they are in the correct order. Signed-off-by: Brian Gesiak --- http-push.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/http-push.c b/http-push.c index d4b40c9..1c722e5 100644 --- a/http-push.c +++ b/http-push.c @@ -1733,7 +1733,7 @@ int main(int argc, char **argv) git_extract_argv0_path(argv[0]); - repo = xcalloc(sizeof(*repo), 1); + repo = xcalloc(1, sizeof(*repo)); argv++; for (i = 1; i < argc; i++, argv++) { -- 2.0.0.rc1.543.gc8042da -- 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 05/15] config.c: rearrange xcalloc arguments
xcalloc takes two arguments: the number of elements and their size. config.c includes several calls to xcalloc that pass the arguments in reverse order: the size of a struct lock_file*, followed by the number to allocate. Rearrgange them so they are in the correct order. Signed-off-by: Brian Gesiak --- config.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/config.c b/config.c index 314d8ee..c3612cc 100644 --- a/config.c +++ b/config.c @@ -1536,7 +1536,7 @@ int git_config_set_multivar_in_file(const char *config_filename, * The lock serves a purpose in addition to locking: the new * contents of .git/config will be written into it. */ - lock = xcalloc(sizeof(struct lock_file), 1); + lock = xcalloc(1, sizeof(struct lock_file)); fd = hold_lock_file_for_update(lock, config_filename, 0); if (fd < 0) { error("could not lock config file %s: %s", config_filename, strerror(errno)); @@ -1791,7 +1791,7 @@ int git_config_rename_section_in_file(const char *config_filename, if (!config_filename) config_filename = filename_buf = git_pathdup("config"); - lock = xcalloc(sizeof(struct lock_file), 1); + lock = xcalloc(1, sizeof(struct lock_file)); out_fd = hold_lock_file_for_update(lock, config_filename, 0); if (out_fd < 0) { ret = error("could not lock config file %s", config_filename); -- 2.0.0.rc1.543.gc8042da -- 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 10/15] imap-send.c: rearrange xcalloc arguments
xcalloc takes two arguments: the number of elements and their size. imap_open_store passes the arguments in reverse order, passing the size of an imap_store*, followed by the number to allocate. Rearrgange them so they are in the correct order. Signed-off-by: Brian Gesiak --- imap-send.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/imap-send.c b/imap-send.c index 0bc6f7f..45230e1 100644 --- a/imap-send.c +++ b/imap-send.c @@ -951,7 +951,7 @@ static struct imap_store *imap_open_store(struct imap_server_conf *srvc) char *arg, *rsp; int s = -1, preauth; - ctx = xcalloc(sizeof(*ctx), 1); + ctx = xcalloc(1, sizeof(*ctx)); ctx->imap = imap = xcalloc(sizeof(*imap), 1); imap->buf.sock.fd[0] = imap->buf.sock.fd[1] = -1; -- 2.0.0.rc1.543.gc8042da -- 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 15/15] transport-helper.c: rearrange xcalloc arguments
xcalloc takes two arguments: the number of elements and their size. transport_helper_init passes the arguments in reverse order, passing the size of a helper_data*, followed by the number to allocate. Rearrgange them so they are in the correct order. Signed-off-by: Brian Gesiak --- transport-helper.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/transport-helper.c b/transport-helper.c index ad72fbd..cf48913 100644 --- a/transport-helper.c +++ b/transport-helper.c @@ -1002,7 +1002,7 @@ static struct ref *get_refs_list(struct transport *transport, int for_push) int transport_helper_init(struct transport *transport, const char *name) { - struct helper_data *data = xcalloc(sizeof(*data), 1); + struct helper_data *data = xcalloc(1, sizeof(*data)); data->name = name; if (getenv("GIT_TRANSPORT_HELPER_DEBUG")) -- 2.0.0.rc1.543.gc8042da -- 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 08/15] hash.h: rearrange xcalloc arguments
xcalloc takes two arguments: the number of elements and their size. prellocate_hash passes the arguments in reverse order, passing the size of a hash table entry, followed by the number of entries. Rearrgange them so they are in the correct order. Signed-off-by: Brian Gesiak --- hash.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/hash.h b/hash.h index 1d43ac0..3b5d9e7 100644 --- a/hash.h +++ b/hash.h @@ -44,7 +44,7 @@ static inline void preallocate_hash(struct hash_table *table, unsigned int elts) { assert(table->size == 0 && table->nr == 0 && table->array == NULL); table->size = elts * 2; - table->array = xcalloc(sizeof(struct hash_table_entry), table->size); + table->array = xcalloc(table->size, sizeof(struct hash_table_entry)); } #endif -- 2.0.0.rc1.543.gc8042da -- 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 12/15] pack-revindex.c: rearrange xcalloc arguments
xcalloc takes two arguments: the number of elements and their size. init_pack_revindex passes the arguments in reverse order, passing the size of a pack_revindex, followed by the number to allocate. Rearrgange them so they are in the correct order. Signed-off-by: Brian Gesiak --- pack-revindex.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pack-revindex.c b/pack-revindex.c index b4d2b35..f84979b 100644 --- a/pack-revindex.c +++ b/pack-revindex.c @@ -50,7 +50,7 @@ static void init_pack_revindex(void) if (!num) return; pack_revindex_hashsz = num * 11; - pack_revindex = xcalloc(sizeof(*pack_revindex), pack_revindex_hashsz); + pack_revindex = xcalloc(pack_revindex_hashsz, sizeof(*pack_revindex)); for (p = packed_git; p; p = p->next) { num = pack_revindex_ix(p); num = - 1 - num; -- 2.0.0.rc1.543.gc8042da -- 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 03/15] builtin/remote.c: rearrange xcalloc arguments
xcalloc takes two arguments: the number of elements and their size. builtin/remote.c includes several calls to xcalloc that pass the arguments in reverse order. Rearrgange them so they are in the correct order. Signed-off-by: Brian Gesiak --- builtin/remote.c | 8 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/builtin/remote.c b/builtin/remote.c index b3ab4cf..9f62021 100644 --- a/builtin/remote.c +++ b/builtin/remote.c @@ -282,7 +282,7 @@ static int config_read_branches(const char *key, const char *value, void *cb) item = string_list_insert(&branch_list, name); if (!item->util) - item->util = xcalloc(sizeof(struct branch_info), 1); + item->util = xcalloc(1, sizeof(struct branch_info)); info = item->util; if (type == REMOTE) { if (info->remote_name) @@ -398,7 +398,7 @@ static int get_push_ref_states(const struct ref *remote_refs, item = string_list_append(&states->push, abbrev_branch(ref->peer_ref->name)); - item->util = xcalloc(sizeof(struct push_info), 1); + item->util = xcalloc(1, sizeof(struct push_info)); info = item->util; info->forced = ref->force; info->dest = xstrdup(abbrev_branch(ref->name)); @@ -433,7 +433,7 @@ static int get_push_ref_states_noquery(struct ref_states *states) states->push.strdup_strings = 1; if (!remote->push_refspec_nr) { item = string_list_append(&states->push, _("(matching)")); - info = item->util = xcalloc(sizeof(struct push_info), 1); + info = item->util = xcalloc(1, sizeof(struct push_info)); info->status = PUSH_STATUS_NOTQUERIED; info->dest = xstrdup(item->string); } @@ -446,7 +446,7 @@ static int get_push_ref_states_noquery(struct ref_states *states) else item = string_list_append(&states->push, _("(delete)")); - info = item->util = xcalloc(sizeof(struct push_info), 1); + info = item->util = xcalloc(1, sizeof(struct push_info)); info->forced = spec->force; info->status = PUSH_STATUS_NOTQUERIED; info->dest = xstrdup(spec->dst ? spec->dst : item->string); -- 2.0.0.rc1.543.gc8042da -- 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 02/15] builtin/ls-remote.c: rearrange xcalloc arguments
xcalloc takes two arguments: the number of elements and their size. cmd_ls_remote passes the arguments in reverse order, passing the size of a char*, followed by the number of char* to be allocated. Rearrgange them so they are in the correct order. Signed-off-by: Brian Gesiak --- builtin/ls-remote.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/builtin/ls-remote.c b/builtin/ls-remote.c index 39e5144..aec1c0c 100644 --- a/builtin/ls-remote.c +++ b/builtin/ls-remote.c @@ -92,7 +92,7 @@ int cmd_ls_remote(int argc, const char **argv, const char *prefix) if (argv[i]) { int j; - pattern = xcalloc(sizeof(const char *), argc - i + 1); + pattern = xcalloc(argc - i + 1, sizeof(const char *)); for (j = i; j < argc; j++) { int len = strlen(argv[j]); char *p = xmalloc(len + 3); -- 2.0.0.rc1.543.gc8042da -- 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 13/15] reflog-walk.c: rearrange xcalloc arguments
xcalloc takes two arguments: the number of elements and their size. reflog-walk.c includes several calls to xcalloc that pass the arguments in reverse order. Rearrgange them so they are in the correct order. Signed-off-by: Brian Gesiak --- reflog-walk.c | 8 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/reflog-walk.c b/reflog-walk.c index b2fbdb2..f8d8f13 100644 --- a/reflog-walk.c +++ b/reflog-walk.c @@ -45,7 +45,7 @@ static int read_one_reflog(unsigned char *osha1, unsigned char *nsha1, static struct complete_reflogs *read_complete_reflog(const char *ref) { struct complete_reflogs *reflogs = - xcalloc(sizeof(struct complete_reflogs), 1); + xcalloc(1, sizeof(struct complete_reflogs)); reflogs->ref = xstrdup(ref); for_each_reflog_ent(ref, read_one_reflog, reflogs); if (reflogs->nr == 0) { @@ -143,7 +143,7 @@ struct reflog_walk_info { void init_reflog_walk(struct reflog_walk_info** info) { - *info = xcalloc(sizeof(struct reflog_walk_info), 1); + *info = xcalloc(1, sizeof(struct reflog_walk_info)); } int add_reflog_for_walk(struct reflog_walk_info *info, @@ -207,7 +207,7 @@ int add_reflog_for_walk(struct reflog_walk_info *info, = reflogs; } - commit_reflog = xcalloc(sizeof(struct commit_reflog), 1); + commit_reflog = xcalloc(1, sizeof(struct commit_reflog)); if (recno < 0) { commit_reflog->recno = get_reflog_recno_by_time(reflogs, timestamp); if (commit_reflog->recno < 0) { @@ -250,7 +250,7 @@ void fake_reflog_parent(struct reflog_walk_info *info, struct commit *commit) return; } - commit->parents = xcalloc(sizeof(struct commit_list), 1); + commit->parents = xcalloc(1, sizeof(struct commit_list)); commit->parents->item = commit_info->commit; } -- 2.0.0.rc1.543.gc8042da -- 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 04/15] commit.c: rearrange xcalloc arguments
xcalloc takes two arguments: the number of elements and their size. reduce_heads passes the arguments in reverse order, passing the size of a commit*, followed by the number of commit* to be allocated. Rearrgange them so they are in the correct order. Signed-off-by: Brian Gesiak --- commit.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/commit.c b/commit.c index 6bf4fe0..0fe685f 100644 --- a/commit.c +++ b/commit.c @@ -1041,7 +1041,7 @@ struct commit_list *reduce_heads(struct commit_list *heads) p->item->object.flags |= STALE; num_head++; } - array = xcalloc(sizeof(*array), num_head); + array = xcalloc(num_head, sizeof(*array)); for (p = heads, i = 0; p; p = p->next) { if (p->item->object.flags & STALE) { array[i++] = p->item; -- 2.0.0.rc1.543.gc8042da -- 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 06/15] diff.c: rearrange xcalloc arguments
xcalloc takes two arguments: the number of elements and their size. diffstat_add passes the arguments in reverse order, passing the size of a diffstat_file*, followed by the number of diffstat_file* to be allocated. Rearrgange them so they are in the correct order. Signed-off-by: Brian Gesiak --- diff.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/diff.c b/diff.c index 635dee2..a71dfde 100644 --- a/diff.c +++ b/diff.c @@ -1360,7 +1360,7 @@ static struct diffstat_file *diffstat_add(struct diffstat_t *diffstat, const char *name_b) { struct diffstat_file *x; - x = xcalloc(sizeof (*x), 1); + x = xcalloc(1, sizeof(*x)); if (diffstat->nr == diffstat->alloc) { diffstat->alloc = alloc_nr(diffstat->alloc); diffstat->files = xrealloc(diffstat->files, -- 2.0.0.rc1.543.gc8042da -- 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 01/15] builtin/add.c: rearrange xcalloc arguments
xcalloc takes two arguments: the number of elements and their size. run_add_interactive passes the arguments in reverse order, passing the size of a char*, followed by the number of char* to be allocated. Rearrgange them so they are in the correct order. Signed-off-by: Brian Gesiak --- builtin/add.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/builtin/add.c b/builtin/add.c index 672adc0..488acf4 100644 --- a/builtin/add.c +++ b/builtin/add.c @@ -248,7 +248,7 @@ int run_add_interactive(const char *revision, const char *patch_mode, int status, ac, i; const char **args; - args = xcalloc(sizeof(const char *), (pathspec->nr + 6)); + args = xcalloc((pathspec->nr + 6), sizeof(const char *)); ac = 0; args[ac++] = "add--interactive"; if (patch_mode) -- 2.0.0.rc1.543.gc8042da -- 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 00/15] Rearrange xcalloc arguments
xcalloc takes two arguments: the number of elements and their size. The vast majority of the Git codebase passes these arguments in the correct order, but there are some exceptions. This patch series corrects those exceptions. Brian Gesiak (15): builtin/add.c: rearrange xcalloc arguments builtin/ls-remote.c: rearrange xcalloc arguments builtin/remote.c: rearrange xcalloc arguments commit.c: rearrange xcalloc arguments config.c: rearrange xcalloc arguments diff.c: rearrange xcalloc arguments hash.c: rearrange xcalloc arguments hash.h: rearrange xcalloc arguments http-push.c: rearrange xcalloc arguments imap-send.c: rearrange xcalloc arguments notes.c: rearrange xcalloc arguments pack-revindex.c: rearrange xcalloc arguments reflog-walk.c: rearrange xcalloc arguments remote.c: rearrange xcalloc arguments transport-helper.c: rearrange xcalloc arguments builtin/add.c | 2 +- builtin/ls-remote.c | 2 +- builtin/remote.c| 8 commit.c| 2 +- config.c| 4 ++-- diff.c | 2 +- hash.c | 2 +- hash.h | 2 +- http-push.c | 2 +- imap-send.c | 2 +- notes.c | 6 +++--- pack-revindex.c | 2 +- reflog-walk.c | 8 remote.c| 2 +- transport-helper.c | 2 +- 15 files changed, 24 insertions(+), 24 deletions(-) -- 2.0.0.rc1.543.gc8042da -- 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/2] api-strbuf.txt: Add docs for _trim and _ltrim
API documentation for strbuf does not document strbuf_trim or strbuf_ltrim. Add documentation for these two functions. Signed-off-by: Brian Gesiak --- Documentation/technical/api-strbuf.txt | 9 + 1 file changed, 9 insertions(+) diff --git a/Documentation/technical/api-strbuf.txt b/Documentation/technical/api-strbuf.txt index 3350d97..4396be9 100644 --- a/Documentation/technical/api-strbuf.txt +++ b/Documentation/technical/api-strbuf.txt @@ -121,10 +121,19 @@ Functions * Related to the contents of the buffer +`strbuf_trim`:: + + Strip whitespace from the beginning and end of a string. + Equivalent to performing `strbuf_rtrim()` followed by `strbuf_ltrim()`. + `strbuf_rtrim`:: Strip whitespace from the end of a string. +`strbuf_ltrim`:: + + Strip whitespace from the beginning of a string. + `strbuf_cmp`:: Compare two buffers. Returns an integer less than, equal to, or greater -- 1.9.2.507.g779792a -- 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/2] strbuf: Use _rtrim and _ltrim in strbuf_trim
strbuf_trim strips whitespace from the end, then the beginning of a strbuf. Those operations are duplicated in strbuf_rtrim and strbuf_ltrim. Replace strbuf_trim implementation with calls to strbuf_rtrim, then strbuf_ltrim. Signed-off-by: Brian Gesiak --- This is tangential to my GSoC project; I noticed the duplication and thought it could be remedied. strbuf.c | 11 ++- 1 file changed, 2 insertions(+), 9 deletions(-) diff --git a/strbuf.c b/strbuf.c index 83caf4a..382cf68 100644 --- a/strbuf.c +++ b/strbuf.c @@ -96,15 +96,8 @@ void strbuf_grow(struct strbuf *sb, size_t extra) void strbuf_trim(struct strbuf *sb) { - char *b = sb->buf; - while (sb->len > 0 && isspace((unsigned char)sb->buf[sb->len - 1])) - sb->len--; - while (sb->len > 0 && isspace(*b)) { - b++; - sb->len--; - } - memmove(sb->buf, b, sb->len); - sb->buf[sb->len] = '\0'; + strbuf_rtrim(sb); + strbuf_ltrim(sb); } void strbuf_rtrim(struct strbuf *sb) { -- 1.9.2.507.g779792a -- 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 in GSoC 2014
Thank you! I'm very excited to be participating in this year's GSoC. Google recommends that students use the next few weeks to get to know their mentors, read documentation, and get up to speed to begin working on their projects. Students have also received instructions on submitting tax forms and other paperwork. Aside from filing all the requisite paperwork, I plan on reading through the extensive set of patches on lock files Michael Haggerty submitted after my initial proposal. I also plan on consulting with my mentor, Jeff King, on some good first steps. By the way, my name is Brian Gesiak. I'm a research student at the University of Tokyo, specializing in parallel and distributed computing. If you have any questions regarding my project, "Unify and Refactor Temporary File Handling", please feel free to contact me via this mailing list, or privately via email. I'm also on GitHub[1] and Twitter[2]. [1] https://github.com/modocache [2] https://twitter.com/modocache - Brian Gesiak On Tue, Apr 22, 2014 at 10:06 AM, Andrew Ardill wrote: > Congrats everyone who was successful in being picked for this year's GSoC. > > Fabian with "Line options for git rebase --interactive" [0] > Brian Gesiak with "Unify and Refactor Temporary File Handling" [1] > Tanay Abhra with "Git configuration API improvements" [2] > > I look forward to seeing how you go! > > [0] > https://www.google-melange.com/gsoc/project/details/google/gsoc2014/bafain/5750085036015616 > [1] > https://www.google-melange.com/gsoc/project/details/google/gsoc2014/modocache/5639274879778816 > [2] > https://www.google-melange.com/gsoc/project/details/google/gsoc2014/tanayabh/5766466041282560 > > Regards, > > Andrew Ardill > -- > 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 -- 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-rebase: Print name of rev when using shorthand
> The concept of "n-th prior checkout" (aka @{-n}) and "immediately > previous checkout" (aka "-") are equivalent, even though the former > may be more generic. > > You seem to be saying that those who understand the former are with > superiour mental capacity in general than those who only know the > latter, and they can always remember where they came from. > > ...have you considered _why_ it is a good thing to show the symbolic > name in the first place? I think I failed to express my point here; I don't think people that use "@{-1}" have superior mental capacity, but rather simply that they are aware of the "@{-n}" method of specifying a previous reference. So in response to the command "git rebase @{-4}", displaying the result "Fast-forwarded HEAD to @{-4}" does not contain any unknown syntax that may confuse them. They may not remember what "@{-4}" refers to, but they are aware of the syntax at least. On the other hand, people who use the "-" shorthand may or may not be aware of the "@{-n}" syntax. In that respect, I think it would be confusing to display "Fast-forwarded HEAD to @{-1}" in response to the command "git rebase -"; the user may not know what "@{-1}" means! Thus my original point was that I felt displaying a symbolic name in response to "git rebase -" was more important than doing so in response to "git rebase @{-1}". The issue isn't about forgetting what "@{-n}" refers to, it's whether the user even knows what "@{-n}" is supposed to mean. But in light of your other comments: >> Furthermore, were we to translate "@{-1}", does that mean we >> should also translate "@{-2}" or prior? > > Surely, why not. If a user is so forgetful to need help remembering > where s/he was immediately before, wouldn't it be more helpful to > give "here is where you were" reminder for older ones to allow them > to double check they specified the right thing and spot possible > mistakes? > > ... > > Giving the symbolic name 'master' is good because it is possible > that the user thought the previous branch was 'frotz', forgetting > that another branch was checked out tentatively in between, and the > user ended up rebasing on top of a wrong branch. Telling what that > previous branch is is a way to help user spot such a potential > mistake. So I am all for making "rebase -" report what concrete > branch the branch was replayed on top of, and consider it an incomplete > improvement if "rebase @{-1}" (or "rebase @{-2}") did not get the > same help---especially when I know that the underlying mechanism you > would use to translate @{-1} back to the concrete branch name is the > same for both cases anyway. I had not originally thought of this, perhaps because I was preoccupied with preventing users from seeing syntax they might not be aware of. But I definitely agree that displaying symbolic names for all "@{-n}" is a good way to prevent user error. > I can buy "that would be a lot more work, and I do not want to do it > (or I do not think I can solve it in a more general way)", though. Perish the thought! :) I will try to re-roll this patch to include symbolic names for "@{-n}". As usual, thanks for the feedback! - Brian Gesiak -- 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
[l10n] date: Note for translators not included in .po files
A note for translators in date.c is not included in git.pot. Namely, the following note from date.c:147 is not included (https://github.com/git/git/blob/v1.9.2/date.c#L147): /* TRANSLATORS: "%s" is " years" */ This is a very useful note for translators (in fact, I think the zh_CN translation for date.c:149 might be a little off because this note was not included. My Mandarin is rusty, but I believe " years, months ago" should be expressed without a comma). According to po/README, the l10n coordinator is responsible for updating the git.pot file. Would it be possible to update it based on v1.9.2 and include the above comment? By the way, I am trying to organize contributors to produce a Japanese localization for Core Git. Currently we have plenty of interest but only two contributors. If you or anyone you know would like to contribute please visit the repository here: https://github.com/modocache/git-po-ja Thanks! - Brian Gesiak -- 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] git-rebase: Print name of rev when using shorthand
The output from a successful invocation of the shorthand command "git rebase -" is something like "Fast-forwarded HEAD to @{-1}", which includes a relative reference to a revision. Other commands that use the shorthand "-", such as "git checkout -", typically display the symbolic name of the revision. Change rebase to output the symbolic name of the revision when using the shorthand. For the example above, the new output is "Fast-forwarded HEAD to master", assuming "@{-1}" is a reference to "master". - Use "git rev-parse" to retreive the name of the rev. - Update the tests in light of this new behavior. Requested-by: John Keeping Signed-off-by: Brian Gesiak --- git-rebase.sh | 8 +++- t/t3400-rebase.sh | 4 +--- 2 files changed, 8 insertions(+), 4 deletions(-) diff --git a/git-rebase.sh b/git-rebase.sh index 2c75e9f..42d34a6 100755 --- a/git-rebase.sh +++ b/git-rebase.sh @@ -455,7 +455,13 @@ then *) upstream_name="$1" if test "$upstream_name" = "-" then - upstream_name="@{-1}" + upstream_name=`git rev-parse --symbolic-full-name @{-1}` + if test -n "$upstream_name" + then + upstream_name=${upstream_name#refs/heads/} + else + upstream_name="@{-1}" + fi fi shift ;; diff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh index 80e0a95..2b99940 100755 --- a/t/t3400-rebase.sh +++ b/t/t3400-rebase.sh @@ -91,7 +91,7 @@ test_expect_success 'rebase from ambiguous branch name' ' test_expect_success 'rebase off of the previous branch using "-"' ' git checkout master && git checkout HEAD^ && - git rebase @{-1} >expect.messages && + git rebase master >expect.messages && git merge-base master HEAD >expect.forkpoint && git checkout master && @@ -100,8 +100,6 @@ test_expect_success 'rebase off of the previous branch using "-"' ' git merge-base master HEAD >actual.forkpoint && test_cmp expect.forkpoint actual.forkpoint && - # the next one is dubious---we may want to say "-", - # instead of @{-1}, in the message test_i18ncmp expect.messages actual.messages ' -- 1.9.0.259.gc5d75e8.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] git-rebase: Print name of rev when using shorthand
Thank you for the feedback! > Imagine the case where there are more than one branches > whose tip points at the commit you came from. > name-rev will not be able to pick correctly which one to report. I see. Yes, you're exactly right; the following demonstrates the problem: $ git checkout -b xylophone master $ git checkout -b aardvark master $ git name-rev --name-only @{-1} # I'd want "xylophone", but this outputs "aardvark" So it appears name-rev is not up to the task here. > I think you would want to use something like: > > upstream_name=$(git rev-parse --symbolic-full-name @{-1}) > if test -n "$upstream" > then > upstream_name=${upstream_name#refs/heads/} > else > upstream_name="@{-1}" > fi > > if the change is to be made at that point in the code. I agree, I will re-roll the patch to use this approach. > I also wonder if "git rebase @{-1}" deserve a similar translation > like you are giving "git rebase -". Personally, I've been using the "-" shorthand with "git checkout" for a year or so, but only learned about "@{-1}" a few months ago. I think those who use "@{-1}" are familiar enough with the concept that they don't need to have the reference translated to a symbolic full name. Users familiar with "-" might not be aware of "@{-1}", however, so I'd prefer not to output it as we are currently. Furthermore, were we to translate "@{-1}", does that mean we should also translate "@{-2}" or prior? I don't think that's the case, but then only translating "@{-1}" would seem inconsistent. >From that point of view I'd prefer to simply translate "-", not "@{-1}". - Brian Gesiak -- 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] git-rebase: Print name of rev when using shorthand
The output from a successful invocation of the shorthand command "git rebase -" is something like "Fast-forwarded HEAD to @{-1}", which includes a relative reference to a revision. Other commands that use the shorthand "-", such as "git checkout -", typically display the symbolic name of the revision. Change rebase to output the symbolic name of the revision when using the shorthand. For the example above, the new output is "Fast-forwarded HEAD to master", assuming "@{-1}" is a reference to "master". - Use "git name-rev" to retreive the name of the rev. - Update the tests in light of this new behavior. Requested-by: John Keeping Signed-off-by: Brian Gesiak --- Previous discussion on this issue: http://article.gmane.org/gmane.comp.version-control.git/244340 git-rebase.sh | 2 +- t/t3400-rebase.sh | 4 +--- 2 files changed, 2 insertions(+), 4 deletions(-) diff --git a/git-rebase.sh b/git-rebase.sh index 2c75e9f..ab0e081 100755 --- a/git-rebase.sh +++ b/git-rebase.sh @@ -455,7 +455,7 @@ then *) upstream_name="$1" if test "$upstream_name" = "-" then - upstream_name="@{-1}" + upstream_name=`git name-rev --name-only @{-1}` fi shift ;; diff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh index 80e0a95..2b99940 100755 --- a/t/t3400-rebase.sh +++ b/t/t3400-rebase.sh @@ -91,7 +91,7 @@ test_expect_success 'rebase from ambiguous branch name' ' test_expect_success 'rebase off of the previous branch using "-"' ' git checkout master && git checkout HEAD^ && - git rebase @{-1} >expect.messages && + git rebase master >expect.messages && git merge-base master HEAD >expect.forkpoint && git checkout master && @@ -100,8 +100,6 @@ test_expect_success 'rebase off of the previous branch using "-"' ' git merge-base master HEAD >actual.forkpoint && test_cmp expect.forkpoint actual.forkpoint && - # the next one is dubious---we may want to say "-", - # instead of @{-1}, in the message test_i18ncmp expect.messages actual.messages ' -- 1.9.0.259.gc5d75e8.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
[PATCH] checkout: Fix grammar in inline comment.
Inline comment had incorrect grammar. Fix grammatical mistakes and reflect actual behavior of the function. Signed-off-by: Brian Gesiak --- builtin/checkout.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/builtin/checkout.c b/builtin/checkout.c index 63151e0..abe1161 100644 --- a/builtin/checkout.c +++ b/builtin/checkout.c @@ -888,8 +888,8 @@ static int parse_branchname_arg(int argc, const char **argv, * * case 3: git checkout [--] * -* (a) If is a commit, that is to -* switch to the branch or detach HEAD at it. As a special case, +* (a) If is a commit, switch to that branch or +* detach HEAD at that commit. As a special case, * if is A...B (missing A or B means HEAD but you can * omit at most one side), and if there is a unique merge base * between A and B, A...B names that merge base. -- 1.9.0.259.gc5d75e8.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 v2] git-rebase: Teach rebase "-" shorthand.
Thank you for the feedback and tweaks! > Is the eye-candy output to the standard output what is the most > interesting during the execution of a rebase? Wouldn't we be > more interested to make sure that we did transplant the history > on the same commit between two cases? I agree. I'll consult the other tests to see how to write such a test. > Can we use `git name-rev` to put the actual name here, so that people > who have not done what they intended can hopefully notice sooner? This sounds like a great idea! Doing so would mirror how `git checkout` behaves; checkout informs the user of which branch they have switched to when using the "-" shorthand: "Switched to branch 'master'". Should I submit a new patch, or reroll this one? - Brian Gesiak -- 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] git-rebase: Teach rebase "-" shorthand.
Teach rebase the same shorthand as checkout and merge; that is, that "-" means "the branch we were previously on". Reported-by: Tim Chase Signed-off-by: Brian Gesiak --- git-rebase.sh | 4 t/t3400-rebase.sh | 11 +++ 2 files changed, 15 insertions(+) diff --git a/git-rebase.sh b/git-rebase.sh index 5f6732b..2c75e9f 100755 --- a/git-rebase.sh +++ b/git-rebase.sh @@ -453,6 +453,10 @@ then test "$fork_point" = auto && fork_point=t ;; *) upstream_name="$1" + if test "$upstream_name" = "-" + then + upstream_name="@{-1}" + fi shift ;; esac diff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh index 6d94b1f..6176754 100755 --- a/t/t3400-rebase.sh +++ b/t/t3400-rebase.sh @@ -88,6 +88,17 @@ test_expect_success 'rebase from ambiguous branch name' ' git rebase master ' +test_expect_success 'rebase using shorthand' ' + git checkout master && + git checkout -b shorthand HEAD^ && + git rebase - 1>shorthand.stdout && + git checkout master && + git branch -D shorthand && + git checkout -b shorthand HEAD^ && + git rebase @{-1} 1>without_shorthand.stdout && + test_i18ncmp without_shorthand.stdout shorthand.stdout +' + test_expect_success 'rebase a single mode change' ' git checkout master && git branch -D topic && -- 1.8.5.2 (Apple Git-48) -- 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] git-rebase: Teach rebase "-" shorthand.
Teach rebase the same shorthand as checkout and merge; that is, that "-" means "the branch we were previously on". Reported-by: Tim Chase Signed-off-by: Brian Gesiak --- git-rebase.sh | 4 t/t3400-rebase.sh | 6 ++ 2 files changed, 10 insertions(+) diff --git a/git-rebase.sh b/git-rebase.sh index 5f6732b..2c75e9f 100755 --- a/git-rebase.sh +++ b/git-rebase.sh @@ -453,6 +453,10 @@ then test "$fork_point" = auto && fork_point=t ;; *) upstream_name="$1" + if test "$upstream_name" = "-" + then + upstream_name="@{-1}" + fi shift ;; esac diff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh index 6d94b1f..00aba9f 100755 --- a/t/t3400-rebase.sh +++ b/t/t3400-rebase.sh @@ -88,6 +88,12 @@ test_expect_success 'rebase from ambiguous branch name' ' git rebase master ' +test_expect_success 'rebase using shorthand' ' + git checkout master + git checkout -b shorthand HEAD^ + GIT_TRACE=1 git rebase - +' + test_expect_success 'rebase a single mode change' ' git checkout master && git branch -D topic && -- 1.8.5.2 (Apple Git-48) -- 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: [GSoC14][RFC] Proposal Draft: Refactor tempfile handling
> Currently the linked list of lockfiles only grows, never shrinks. Once > an object has been linked into the list, there is no way to remove it > again even after the lock has been released. So if a lock needs to be > created dynamically at a random place in the code, its memory is > unavoidably leaked. Ah yes, I see. I think a good example is config.git_config_set_multivar_in_file, which even contains a comment detailing the problem: "Since lockfile.c keeps a linked list of all created lock_file structures, it isn't safe to free(lock). It's better to just leave it hanging around." > But I have a feeling that if we want to use a similar mechanism to > handle all temporary files (of which there can be more), then it would > be a good idea to lift this limitation. It will require some care, > though, to make sure that record removal is done in a way that is > threadsafe and safe in the event of all expected kinds of process death. It sounds like a threadsafe linked-list with an interface to manually remove elements from the list is the solution here; does that sound reasonable? Ensuring thread safety without sacrificing readability is probably more difficult than it sounds, but I don't think it's impossible. I'll add some more details on this to my proposal[1]. Thank you! - Brian Gesiak [1] https://www.google-melange.com/gsoc/proposal/review/student/google/gsoc2014/modocache/5629499534213120 -- 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: [GSoC14][RFC] Proposal Draft: Refactor tempfile handling
Excellent, thank you very much for the feedback, Jeff! It was very helpful and encouraging. I've done some more research based on your comments. > Once the logic is extracted into a nice API, there are > several other places that can use it, too: ... I've found the following four areas so far: 1. lockfile.lock_file 2. git-compat-util.odb_mkstemp 3. git-compat-util.odb_pack_keep 4. diff.prepare_temp_file Tons of files use (1) and (2). (3) is less common, and (4) is only used for external diffs. > the shallow_XX tempfiles I'm not sure I was able to find this one. Are you referring to the lock files used when fetching, such as in fetch-pack.c? > What are the mismatches in how lockfiles and object files are > handled? E.g., how do we finalize them into place? How should > the API be designed to minimize race conditions (e.g., if we > get a signal delivered while we are committing or cleaning up a file)? I'd say the biggest difference between lockfiles and object files is that tempfile methods like odb_mkstemp need to know the location of the object directory. Aside from that, lockfiles and the external diff files appear to be cleaned up at exit, while temporary object files tend to have a more finely controlled lifecycle. I'm still investigating this aspect of the proposal, though. One question, though: the idea on the ideas page specifies that temporary pack and object files may "optionally" be cleaned up in case of error during program execution. How will users specify their preference? I think the API for creating temporary files should allow cleanup options to be specified on a per-file basis. That way each part of the program that creates tempfiles can specify a different config value to determine the cleanup policy. Thanks for all your help so far! - Brian Gesiak PS: I'm maintaining a working draft of my proposal here, in case anyone wants to offer any feedback prior to its submission: https://gist.github.com/modocache/9434914 On Tue, Mar 4, 2014 at 7:42 AM, Jeff King wrote: > On Sun, Mar 02, 2014 at 06:04:39AM +0900, Brian Gesiak wrote: > >> My name is Brian Gesiak. I'm a research student at the University of >> Tokyo, and I'm hoping to participate in this year's Google Summer of >> Code by contributing to Git. I'm a longtime user, first-time >> contributor--some of you may have noticed my "microproject" >> patches.[1][2] > > Yes, we did notice them. Thanks, and welcome. :) > >> The ideas page points out that while lock files are closed and >> unlinked[3] when the program exits[4], object pack files implement >> their own brand of temp file creation and deletion. This >> implementation doesn't share the same guarantees as lock files--it is >> possible that the program terminates before the temp file is >> unlinked.[5] >> >> Lock file references are stored in a linked list. When the program >> exits, this list is traversed and each file is closed and unlinked. It >> seems to me that this mechanism is appropriate for temp files in >> general, not just lock files. Thus, my proposal would be to extract >> this logic into a separate module--tempfile.h, perhaps. Lock and >> object files would share the tempfile implementation. >> >> That is, both object and lock temp files would be stored in a linked >> list, and all of these would be deleted at program exit. > > Yes, I think this is definitely the right way to go. We should be able > to unify the tempfile handling for all of git. Once the logic is > extracted into a nice API, there are several other places that can use > it, too: > > - the external diff code creates tempfiles and uses its own cleanup > routines > > - the shallow_XX tempfiles (these are not cleaned right now, > though I sent a patch recently for them to do their own cleanup) > > Those are just off the top of my head. There may be other spots, too. > > It is worth thinking in your proposal about some of the things that the > API will want to handle. What are the mismatches in how lockfiles and > object files are handled? E.g., how do we finalize them into place? > How should the API be designed to minimize race conditions (e.g., if we > get a signal delivered while we are committing or cleaning up a file)? > >> Please let me know if this seems like it would make for an interesting >> proposal, or if perhaps there is something I am overlooking. Any >> feedback at all would be appreciated. Thank you! > > You definitely have a grasp of what the project is aiming for, and which > areas need to be touched. > > -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] t3200-branch: test setting branch as own upstream
No test asserts that "git branch -u refs/heads/my-branch my-branch" emits a warning. Add a test that does so. Signed-off-by: Brian Gesiak --- t/t3200-branch.sh | 10 ++ 1 file changed, 10 insertions(+) diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh index fcdb867..e6d4015 100755 --- a/t/t3200-branch.sh +++ b/t/t3200-branch.sh @@ -507,6 +507,16 @@ EOF test_cmp expected actual ' +test_expect_success '--set-upstream-to shows warning if used to set branch as own upstream' ' + git branch --set-upstream-to refs/heads/my13 my13 2>actual && + cat >expected <expect < 1117150200 + branch: Created from master -- 1.8.3.4 (Apple Git-47) -- 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
[GSoC14][RFC] Proposal Draft: Refactor tempfile handling
Hello all, My name is Brian Gesiak. I'm a research student at the University of Tokyo, and I'm hoping to participate in this year's Google Summer of Code by contributing to Git. I'm a longtime user, first-time contributor--some of you may have noticed my "microproject" patches.[1][2] I'd like to gather some information on one of the GSoC ideas posted on the ideas page. Namely, I'm interested in refactoring the way tempfiles are cleaned up. The ideas page points out that while lock files are closed and unlinked[3] when the program exits[4], object pack files implement their own brand of temp file creation and deletion. This implementation doesn't share the same guarantees as lock files--it is possible that the program terminates before the temp file is unlinked.[5] Lock file references are stored in a linked list. When the program exits, this list is traversed and each file is closed and unlinked. It seems to me that this mechanism is appropriate for temp files in general, not just lock files. Thus, my proposal would be to extract this logic into a separate module--tempfile.h, perhaps. Lock and object files would share the tempfile implementation. That is, both object and lock temp files would be stored in a linked list, and all of these would be deleted at program exit. I'm very enthused about this project--I think it has it all: - Tangible benefits for the end-user - Reduced complexity in the codebase - Ambitious enough to be interesting - Small enough to realistically be completed in a summer Please let me know if this seems like it would make for an interesting proposal, or if perhaps there is something I am overlooking. Any feedback at all would be appreciated. Thank you! - Brian Gesiak [1] http://thread.gmane.org/gmane.comp.version-control.git/242891 [2] http://thread.gmane.org/gmane.comp.version-control.git/242893 [3] https://github.com/git/git/blob/v1.9.0/lockfile.c#L18 [4] https://github.com/git/git/blob/v1.9.0/lockfile.c#L143 [5] https://github.com/git/git/blob/v1.9.0/pack-write.c#L350 -- 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/3] branch: die when setting branch as own upstream
Sorry for the multiple patches--I noticed the commit author was off in the first one. This patch converts the warning to an error, should it be decided that it's prudent to do so (I'm in favor of doing so). If not, I think the other two patches I submitted are good to merge. Thanks for all the feedback so far! - Brian Gesiak On Sat, Mar 1, 2014 at 9:23 PM, Brian Gesiak wrote: > Branch set as own upstream using one of the following commands returns > immediately with an exit code of 0: > > - `git branch --set-upstream-to foo refs/heads/foo` > - `git branch --force --track foo foo` > > Since neither of these actions currently set the upstream, an exit code > of 0 is misleading. Instead, exit with a status code indicating failure > by using the die function. > > Signed-off-by: Brian Gesiak > --- > branch.c | 9 ++--- > t/t3200-branch.sh | 6 +++--- > 2 files changed, 5 insertions(+), 10 deletions(-) > > diff --git a/branch.c b/branch.c > index e163f3c..9bac8b5 100644 > --- a/branch.c > +++ b/branch.c > @@ -54,13 +54,8 @@ void install_branch_config(int flag, const char *local, > const char *origin, cons > struct strbuf key = STRBUF_INIT; > int rebasing = should_setup_rebase(origin); > > - if (shortname > - && !strcmp(local, shortname) > - && !origin) { > - warning(_("Not setting branch %s as its own upstream."), > - local); > - return; > - } > + if (shortname && !strcmp(local, shortname) && !origin) > + die(_("Not setting branch %s as its own upstream."), local); > > strbuf_addf(&key, "branch.%s.remote", local); > git_config_set(key.buf, origin ? origin : "."); > diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh > index 6164126..3ac493f 100755 > --- a/t/t3200-branch.sh > +++ b/t/t3200-branch.sh > @@ -507,10 +507,10 @@ EOF > test_cmp expected actual > ' > > -test_expect_success '--set-upstream-to shows warning if used to set branch > as own upstream' ' > - git branch --set-upstream-to refs/heads/my13 my13 2>actual && > +test_expect_success '--set-upstream-to fails if used to set branch as own > upstream' ' > + test_must_fail git branch --set-upstream-to refs/heads/my13 my13 > 2>actual && > cat >expected < -warning: Not setting branch my13 as its own upstream. > +fatal: Not setting branch my13 as its own upstream. > EOF > test_i18ncmp expected actual > ' > -- > 1.8.3.4 (Apple Git-47) > -- 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/3] branch: die when setting branch as own upstream
Branch set as own upstream using one of the following commands returns immediately with an exit code of 0: - `git branch --set-upstream-to foo refs/heads/foo` - `git branch --force --track foo foo` Since neither of these actions currently set the upstream, an exit code of 0 is misleading. Instead, exit with a status code indicating failure by using the die function. Signed-off-by: Brian Gesiak --- branch.c | 9 ++--- t/t3200-branch.sh | 6 +++--- 2 files changed, 5 insertions(+), 10 deletions(-) diff --git a/branch.c b/branch.c index e163f3c..9bac8b5 100644 --- a/branch.c +++ b/branch.c @@ -54,13 +54,8 @@ void install_branch_config(int flag, const char *local, const char *origin, cons struct strbuf key = STRBUF_INIT; int rebasing = should_setup_rebase(origin); - if (shortname - && !strcmp(local, shortname) - && !origin) { - warning(_("Not setting branch %s as its own upstream."), - local); - return; - } + if (shortname && !strcmp(local, shortname) && !origin) + die(_("Not setting branch %s as its own upstream."), local); strbuf_addf(&key, "branch.%s.remote", local); git_config_set(key.buf, origin ? origin : "."); diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh index 6164126..3ac493f 100755 --- a/t/t3200-branch.sh +++ b/t/t3200-branch.sh @@ -507,10 +507,10 @@ EOF test_cmp expected actual ' -test_expect_success '--set-upstream-to shows warning if used to set branch as own upstream' ' - git branch --set-upstream-to refs/heads/my13 my13 2>actual && +test_expect_success '--set-upstream-to fails if used to set branch as own upstream' ' + test_must_fail git branch --set-upstream-to refs/heads/my13 my13 2>actual && cat >expected <http://vger.kernel.org/majordomo-info.html
[PATCH v2] branch: die when setting branch as own upstream
From: modocache Branch set as own upstream using one of the following commands returns immediately with an exit code of 0: - `git branch --set-upstream-to foo refs/heads/foo` - `git branch --force --track foo foo` Since neither of these actions currently set the upstream, an exit code of 0 is misleading. Instead, exit with a status code indicating failure by using the die function. Signed-off-by: Brian Gesiak --- branch.c | 9 ++--- t/t3200-branch.sh | 6 +++--- 2 files changed, 5 insertions(+), 10 deletions(-) diff --git a/branch.c b/branch.c index e163f3c..9bac8b5 100644 --- a/branch.c +++ b/branch.c @@ -54,13 +54,8 @@ void install_branch_config(int flag, const char *local, const char *origin, cons struct strbuf key = STRBUF_INIT; int rebasing = should_setup_rebase(origin); - if (shortname - && !strcmp(local, shortname) - && !origin) { - warning(_("Not setting branch %s as its own upstream."), - local); - return; - } + if (shortname && !strcmp(local, shortname) && !origin) + die(_("Not setting branch %s as its own upstream."), local); strbuf_addf(&key, "branch.%s.remote", local); git_config_set(key.buf, origin ? origin : "."); diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh index 6164126..3ac493f 100755 --- a/t/t3200-branch.sh +++ b/t/t3200-branch.sh @@ -507,10 +507,10 @@ EOF test_cmp expected actual ' -test_expect_success '--set-upstream-to shows warning if used to set branch as own upstream' ' - git branch --set-upstream-to refs/heads/my13 my13 2>actual && +test_expect_success '--set-upstream-to fails if used to set branch as own upstream' ' + test_must_fail git branch --set-upstream-to refs/heads/my13 my13 2>actual && cat >expected <http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] t3200-branch: test setting branch as own upstream
> I just don't want to regress somebody else's workflow due > to my lack of imagination. This makes a lot of sense to me, although as-is the function emits a warning and returns immediately (although with a successful status code), so I'm also stumped as to what kind of workflow this would be included in. In any case, if the jury's out on this one, I suppose the two patches I submitted are good to merge? Part of me thinks the bump from warning to error belongs in its own patch anyway. - Brian Gesiak -- 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 1/2] t3200-branch: test setting branch as own upstream
> If you feel like continuing on this series, converting the warning() > into a die() would be a much more productive use of time (but if you > don't, I do not see any reason not to take the patches you've posted). I'd be happy to keep working on this. In fact, I think I have a patch for this ready. But just to clarify: > I notice that the warning comes from install_branch_config, which gets > used both for "branch -u", but also in the "side effect" case I > mentioned above. Is it possible to trigger this as part of such a case? > I think maybe "git branch -f --track foo foo" would do it. If so, we > should perhaps include a test that it does not break if we upgrade the > "-u" case to an error. Do you mean that install_branch_config should continue to emit a warning in the "side effect" case? I'm not sure I agree--how is "git branch -f --track foo foo" less erroneous than "git branch -u foo refs/heads/foo"? Perhaps I'm missing some insight on how "--track" is used. The tests appear to already cover all instances in which install_branch_config is called, and bumping the warning to an error does not cause any test failures. - Brian Gesiak -- 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 1/2] t3200-branch: test setting branch as own upstream
> Hmm. Looks like it is only a problem if you are calling a shell function > (since it is the shell function's trace output you are seeing). So this > test would be OK as-is Indeed, this test passes when run locally, even using "sh -x". I would be in favor of using test_i18ngrep, but it seems like this test file overwhelmingly uses test_(i18n)cmp, even when inspecting stderr output. Making double-sure that all tests pass when run with "sh -x" seems like a larger endeavor. Of course, I'd be happy to submit several patches if there's support for such a change. But as Peff points out it will be a large diff. - Brian Gesiak On Fri, Feb 28, 2014 at 4:26 PM, Jeff King wrote: > On Fri, Feb 28, 2014 at 02:14:01AM -0500, Jeff King wrote: > >> I didn't think we bothered to make "sh -x" work robustly. I don't mind >> if we do, but "git grep -E 'test_(i18n)?cmp .*err" shows many potential >> problem spots. > > Just for fun: > > cd t > make SHELL_PATH="sh -x" prove > > causes 326 test failures across 43 scripts. That's slightly misleading, > because 200 of the failures are all in t0008, and updating one function > would probably clear up all of them. > > -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 v2 2/2] branch: use skip_prefix
The install_branch_config function reimplemented the skip_prefix function inline. Use skip_prefix function instead for brevity. Reported-by: Michael Haggerty Signed-off-by: Brian Gesiak --- branch.c | 18 +- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/branch.c b/branch.c index 723a36b..e163f3c 100644 --- a/branch.c +++ b/branch.c @@ -1,3 +1,4 @@ +#include "git-compat-util.h" #include "cache.h" #include "branch.h" #include "refs.h" @@ -49,12 +50,11 @@ static int should_setup_rebase(const char *origin) void install_branch_config(int flag, const char *local, const char *origin, const char *remote) { - const char *shortname = remote + 11; - int remote_is_branch = starts_with(remote, "refs/heads/"); + const char *shortname = skip_prefix(remote, "refs/heads/"); struct strbuf key = STRBUF_INIT; int rebasing = should_setup_rebase(origin); - if (remote_is_branch + if (shortname && !strcmp(local, shortname) && !origin) { warning(_("Not setting branch %s as its own upstream."), @@ -77,29 +77,29 @@ void install_branch_config(int flag, const char *local, const char *origin, cons strbuf_release(&key); if (flag & BRANCH_CONFIG_VERBOSE) { - if (remote_is_branch && origin) + if (shortname && origin) printf_ln(rebasing ? _("Branch %s set up to track remote branch %s from %s by rebasing.") : _("Branch %s set up to track remote branch %s from %s."), local, shortname, origin); - else if (remote_is_branch && !origin) + else if (shortname && !origin) printf_ln(rebasing ? _("Branch %s set up to track local branch %s by rebasing.") : _("Branch %s set up to track local branch %s."), local, shortname); - else if (!remote_is_branch && origin) + else if (!shortname && origin) printf_ln(rebasing ? _("Branch %s set up to track remote ref %s by rebasing.") : _("Branch %s set up to track remote ref %s."), local, remote); - else if (!remote_is_branch && !origin) + else if (!shortname && !origin) printf_ln(rebasing ? _("Branch %s set up to track local ref %s by rebasing.") : _("Branch %s set up to track local ref %s."), local, remote); else - die("BUG: impossible combination of %d and %p", - remote_is_branch, origin); + die("BUG: impossible combination of %p and %p", + shortname, origin); } } -- 1.8.3.4 (Apple Git-47) -- 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 1/2] t3200-branch: test setting branch as own upstream
No test asserts that "git branch -u refs/heads/my-branch my-branch" emits a warning. Add a test that does so. Signed-off-by: Brian Gesiak --- t/t3200-branch.sh | 8 1 file changed, 8 insertions(+) diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh index fcdb867..6164126 100755 --- a/t/t3200-branch.sh +++ b/t/t3200-branch.sh @@ -507,6 +507,14 @@ EOF test_cmp expected actual ' +test_expect_success '--set-upstream-to shows warning if used to set branch as own upstream' ' + git branch --set-upstream-to refs/heads/my13 my13 2>actual && + cat >expected <expect < 1117150200 + branch: Created from master -- 1.8.3.4 (Apple Git-47) -- 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 1/2] t3200-branch: test setting branch as own upstream
> For an operation like "git branch foo origin" where setting up the > tracking is a side effect, a warning makes sense. But the sole purpose > of the command above is to set the upstream, and we didn't do it; should > this warning actually be upgraded to an error? I agree. I originally wrote the test using test_expect_failure--imagine my surprise when the exit status was 0, despite the fact that the upstream wasn't set! > This should use test_i18ncmp, as the string you are matching is > internationalized. Patch is on the way, just waiting for the tests to complete. Thanks for pointing that out! Also, sorry if it's in the Makefile somewhere, but is there an easy way to run just a single test file in the t directory? - Brian Gesiak -- 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/2] branch: use skip_prefix
From: modocache The install_branch_config function reimplemented the skip_prefix function inline. Use skip_prefix function instead for brevity. Signed-off-by: Brian Gesiak Reported-by: Michael Haggerty --- branch.c | 18 +- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/branch.c b/branch.c index 723a36b..e163f3c 100644 --- a/branch.c +++ b/branch.c @@ -1,3 +1,4 @@ +#include "git-compat-util.h" #include "cache.h" #include "branch.h" #include "refs.h" @@ -49,12 +50,11 @@ static int should_setup_rebase(const char *origin) void install_branch_config(int flag, const char *local, const char *origin, const char *remote) { - const char *shortname = remote + 11; - int remote_is_branch = starts_with(remote, "refs/heads/"); + const char *shortname = skip_prefix(remote, "refs/heads/"); struct strbuf key = STRBUF_INIT; int rebasing = should_setup_rebase(origin); - if (remote_is_branch + if (shortname && !strcmp(local, shortname) && !origin) { warning(_("Not setting branch %s as its own upstream."), @@ -77,29 +77,29 @@ void install_branch_config(int flag, const char *local, const char *origin, cons strbuf_release(&key); if (flag & BRANCH_CONFIG_VERBOSE) { - if (remote_is_branch && origin) + if (shortname && origin) printf_ln(rebasing ? _("Branch %s set up to track remote branch %s from %s by rebasing.") : _("Branch %s set up to track remote branch %s from %s."), local, shortname, origin); - else if (remote_is_branch && !origin) + else if (shortname && !origin) printf_ln(rebasing ? _("Branch %s set up to track local branch %s by rebasing.") : _("Branch %s set up to track local branch %s."), local, shortname); - else if (!remote_is_branch && origin) + else if (!shortname && origin) printf_ln(rebasing ? _("Branch %s set up to track remote ref %s by rebasing.") : _("Branch %s set up to track remote ref %s."), local, remote); - else if (!remote_is_branch && !origin) + else if (!shortname && !origin) printf_ln(rebasing ? _("Branch %s set up to track local ref %s by rebasing.") : _("Branch %s set up to track local ref %s."), local, remote); else - die("BUG: impossible combination of %d and %p", - remote_is_branch, origin); + die("BUG: impossible combination of %p and %p", + shortname, origin); } } -- 1.8.3.4 (Apple Git-47) -- 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/2] t3200-branch: test setting branch as own upstream
From: modocache No test asserts that "git branch -u refs/heads/my-branch my-branch" emits a warning. Add a test that does so. Signed-off-by: Brian Gesiak --- t/t3200-branch.sh | 8 1 file changed, 8 insertions(+) diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh index fcdb867..f70b96f 100755 --- a/t/t3200-branch.sh +++ b/t/t3200-branch.sh @@ -507,6 +507,14 @@ EOF test_cmp expected actual ' +test_expect_success '--set-upstream-to shows warning if used to set branch as own upstream' ' + git branch --set-upstream-to refs/heads/my13 my13 2>actual && + cat >expected <expect < 1117150200 + branch: Created from master -- 1.8.3.4 (Apple Git-47) -- 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