Re: BUG: git request-pull broken for plain branches
Hi Junio, On Wed, Jun 25, 2014 at 01:41:23PM -0700, Junio C Hamano wrote: > Uwe Kleine-König writes: > > On Wed, Jun 25, 2014 at 05:05:51AM -0700, Linus Torvalds wrote: > >> On Wed, Jun 25, 2014 at 2:55 AM, Uwe Kleine-König > >> wrote: > >> > > >> > $ git rev-parse HEAD > >> > 9e065e4a5a58308f1a0da4bb80b830929dfa90b3 > >> > $ git ls-remote origin | grep > >> > 9e065e4a5a58308f1a0da4bb80b830929dfa90b3 > >> > 9e065e4a5a58308f1a0da4bb80b830929dfa90b3 > >> > refs/heads/ukl/for-mainline > >> > $ git request-pull origin/master origin HEAD > /dev/null > >> > warn: No match for commit > >> > 9e065e4a5a58308f1a0da4bb80b830929dfa90b3 found at origin > >> > warn: Are you sure you pushed 'HEAD' there? > >> > >> Notice how "HEAD" does not match. > >> > >> The error message is perhaps misleading. It's not enough to match the > >> commit. You need to match the branch name too. git used to guess the > >> branch name (from the commit), and it often guessed wrongly. So now > >> they need to match. > >> > >> So you should do > >> > >> git request-pull origin/master origin ukl/for-mainline > >> > >> to let request-pull know that you're requesting a pull for > >> "ukl/for-mainline". > > Or > > git request-pull origin/master origin HEAD:ukl/for-mainline > > I am not Linus, and do not speak for him, but FWIW here are some > comments on the ideas. > > > I liked git guessing the branch name, maybe we can teach it to guess a > > bit better than it did before 2.0? Something like: > > > > - if there is a unique match on the remote side, use it. > > I am on the fence but slightly leaning to the negative side on this > one. The branch/ref the object was took from when "git pull" is run > does matter, because the name is recorded in the merge summary, so > we cannot say "There are refs that happen to point at the object you > wanted to be pulled, so we'll pick one of them let the integrator > pull from that ref we randomly chose" is not something we would > want. "If there is a unique one, that must be the one the user has > meant; there is nothing else possible" feels like a strong argument, > and I was actually contemplating about doing an enhancement on top > of Linus's original myself along that line, back when we queued that > patch exactly for that reason. > > But a counter-argument, in the context of Linus's change in question > being primarily to avoid end-user mistakes resulting in a bogus > request, is that the ref on the remote that happens to match the > object but is different from what the user named may be a totally > unrelated branch before the user actually has pushed the set of > changes the request is going to ask to be pulled. The mistake that > this extra strictness tries to avoid is to compose request-pull > before pushing what to be pulled and then forgetting to push. Sounds sensible. Then the enhancements that come to my mind are: Change git-request-pull to explicitly take a remote ref as end. This makes sure that it is actually there and the right remote name is picked. Don't require to specify a local ref even if there is no local matching ref, just use the remote sha1 to generate the diffstat and summary. Another thing I'd like to have is to make git-request-pull not generate the usual output if there is no match. Optionally introduce a -f to get back the (maybe bogus) output; in this case a local rev would be needed. Best regards Uwe -- Pengutronix e.K. | Uwe Kleine-König| Industrial Linux Solutions | http://www.pengutronix.de/ | -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC/PATCH V2] branch.c: replace git_config with git_config_get_string
On 6/25/2014 10:15 AM, Eric Sunshine wrote:
> On Mon, Jun 23, 2014 at 6:41 AM, Tanay Abhra wrote:
>> diff --git a/branch.c b/branch.c
>> index 660097b..c9a2a0d 100644
>> --- a/branch.c
>> +++ b/branch.c
>> @@ -140,33 +140,25 @@ static int setup_tracking(const char *new_ref, const
>> char *orig_ref,
>> return 0;
>> }
>>
>> -struct branch_desc_cb {
>> +struct branch_desc {
>> const char *config_name;
>> const char *value;
>> };
>
> What is the purpose of retaining this structure? Following your
> changes, it is never used outside of read_branch_desc(), and
> 'config_name' and 'value' would be more naturally declared as
> variables local to that function.
Done. :)
>
>> -static int read_branch_desc_cb(const char *var, const char *value, void *cb)
>> -{
>> - struct branch_desc_cb *desc = cb;
>> - if (strcmp(desc->config_name, var))
>> - return 0;
>> - free((char *)desc->value);
>> - return git_config_string(&desc->value, var, value);
>> -}
>> -
>> int read_branch_desc(struct strbuf *buf, const char *branch_name)
>> {
>> - struct branch_desc_cb cb;
>> + const char *value = NULL;
>> + struct branch_desc desc;
>> struct strbuf name = STRBUF_INIT;
>> strbuf_addf(&name, "branch.%s.description", branch_name);
>> - cb.config_name = name.buf;
>> - cb.value = NULL;
>> - if (git_config(read_branch_desc_cb, &cb) < 0) {
>> + desc.config_name = name.buf;
>> + desc.value = NULL;
>> + git_config_get_string(desc.config_name, &value);
>> + if (git_config_string(&desc.value, desc.config_name, value) < 0) {
>
> Although it works in this case, it's somewhat ugly that you ignore the
> return value of git_config_get_string(), and a person reading the code
> has to spend extra time digging into git_config_string() to figure out
> why this is safe. If might be clearer for future readers by rephrasing
> like this:
>
> if (git_config_get_string(desc.config_name, &value) < 0 ||
> git_config_string(&desc.value, desc.config_name, value) < 0) {
>
Noted, also didn't the old code leak desc.value as it was xstrduped
by git_config_string()? Thanks for the review.
>> strbuf_release(&name);
>> return -1;
>> }
>> - if (cb.value)
>> - strbuf_addstr(buf, cb.value);
>> + strbuf_addstr(buf, desc.value);
>> strbuf_release(&name);
>> return 0;
>> }
>> --
>> 1.9.0.GIT
>
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC/PATCH] imap-send.c: replace git_config with git_config_get_string
On 6/25/2014 12:39 PM, Eric Sunshine wrote:
> On Mon, Jun 23, 2014 at 6:41 AM, Tanay Abhra wrote:
>> Use git_config_get_string instead of git_config to take advantage of
>> the config hash-table api which provides a cleaner control flow.
>
> You may want to mention as a side-note the slight behavior change
> introduced by this patch. The original code complained about any
> unknown boolean "imap.*" key, whereas the new code does not.
>
Also, my code is error prone. Previous one had all NULL values returned
as config_non_boolean. But, now I have to add a NULL check to every strdup
in the code.
More below,
>
>> Signed-off-by: Tanay Abhra
>> ---
>> imap-send.c | 68
>> ++---
>> 1 file changed, 29 insertions(+), 39 deletions(-)
>>
>> diff --git a/imap-send.c b/imap-send.c
>> index 83a6ed2..87bd418 100644
>> --- a/imap-send.c
>> +++ b/imap-send.c
>> @@ -1326,47 +1326,37 @@ static int split_msg(struct strbuf *all_msgs, struct
>> strbuf *msg, int *ofs)
>>
>> static char *imap_folder;
>>
>> -static int git_imap_config(const char *key, const char *val, void *cb)
>> +static void git_imap_config(void)
>> {
>> - char imap_key[] = "imap.";
>> -
>> - if (strncmp(key, imap_key, sizeof imap_key - 1))
>> - return 0;
>> -
>> - key += sizeof imap_key - 1;
>> -
>> - /* check booleans first, and barf on others */
>> - if (!strcmp("sslverify", key))
>> - server.ssl_verify = git_config_bool(key, val);
>> - else if (!strcmp("preformattedhtml", key))
>> - server.use_html = git_config_bool(key, val);
>> - else if (!val)
>> - return config_error_nonbool(key);
>> -
>> - if (!strcmp("folder", key)) {
>> - imap_folder = xstrdup(val);
>> - } else if (!strcmp("host", key)) {
>> - if (starts_with(val, "imap:"))
>> - val += 5;
>> - else if (starts_with(val, "imaps:")) {
>> - val += 6;
>> + const char *value;
>
> Observation: If you name this variable 'val', which is the name of the
> argument to the function in the original code, you will get a slightly
> smaller and more readable diff.
Noted.
>
>> + if (!git_config_get_string("imap.sslverify", &value))
>> + server.ssl_verify = git_config_bool("sslverify", value);
>
> I realize that you are just replicating the behavior of the original
> code, but the error message emitted here for a non-bool value is less
> than desirable since it throws away context (namely, the "imap."
> prefix). You can improve the message, and help the user resolve the
> error more quickly, by presenting the full configuration key (namely,
> "imap.sslverify"). Such a change would deserve mention in the commit
> message. Alternately, it could be fixed in a follow-up patch.
>
Yes, I thought so also when writing the patch. Will change it in the next
iteration.
Thanks.
Tanay Abhra.
>> + if (!git_config_get_string("imap.preformattedhtml", &value))
>> + server.use_html = git_config_bool("preformattedhtml", value);
>
> Ditto regarding error message: "imap.preformattedhtml"
>
>> + if (!git_config_get_string("imap.folder", &value))
>> + imap_folder = xstrdup(value);
>> + if (!git_config_get_string("imap.host", &value)) {
>> + if (starts_with(value, "imap:"))
>> + value += 5;
>> + else if (starts_with(value, "imaps:")) {
>> + value += 6;
>> server.use_ssl = 1;
>> }
>> - if (starts_with(val, "//"))
>> - val += 2;
>> - server.host = xstrdup(val);
>> - } else if (!strcmp("user", key))
>> - server.user = xstrdup(val);
>> - else if (!strcmp("pass", key))
>> - server.pass = xstrdup(val);
>> - else if (!strcmp("port", key))
>> - server.port = git_config_int(key, val);
>> - else if (!strcmp("tunnel", key))
>> - server.tunnel = xstrdup(val);
>> - else if (!strcmp("authmethod", key))
>> - server.auth_method = xstrdup(val);
>> -
>> - return 0;
>> + if (starts_with(value, "//"))
>> + value += 2;
>> + server.host = xstrdup(value);
>> + }
>> + if (!git_config_get_string("imap.user", &value))
>> + server.user = xstrdup(value);
>> + if (!git_config_get_string("imap.pass", &value))
>> + server.pass = xstrdup(value);
>> + if (!git_config_get_string("imap.port", &value))
>> + server.port = git_config_int("port", value);
>
> Same regarding diagnostic: "imap.port"
>
>> + if (!git_config_get_string("imap.tunnel", &value))
>> + server.tunnel = xstrdup(value);
>> + if (!git_config_get_string("imap.authm
Re: [RFC/PATCH] notes-util.c: replace git_config with git_config_get_string
On 6/25/2014 1:24 PM, Eric Sunshine wrote:
> On Mon, Jun 23, 2014 at 6:41 AM, Tanay Abhra wrote:
>> Use git_config_get_string instead of git_config to take advantage of
>> the config hash-table api which provides a cleaner control flow.
>>
>> Signed-off-by: Tanay Abhra
>> ---
>> notes-utils.c | 31 +++
>> 1 file changed, 15 insertions(+), 16 deletions(-)
>>
>> diff --git a/notes-utils.c b/notes-utils.c
>> index a0b1d7b..fdc9912 100644
>> --- a/notes-utils.c
>> +++ b/notes-utils.c
>> @@ -68,22 +68,23 @@ static combine_notes_fn parse_combine_notes_fn(const
>> char *v)
>> return NULL;
>> }
>>
>> -static int notes_rewrite_config(const char *k, const char *v, void *cb)
>> +static void notes_rewrite_config(struct notes_rewrite_cfg *c)
>> {
>> - struct notes_rewrite_cfg *c = cb;
>> - if (starts_with(k, "notes.rewrite.") && !strcmp(k+14, c->cmd)) {
>> - c->enabled = git_config_bool(k, v);
>> - return 0;
>> - } else if (!c->mode_from_env && !strcmp(k, "notes.rewritemode")) {
>> + struct strbuf key = STRBUF_INIT;
>> + const char *v;
>> + strbuf_addf(&key, "notes.rewrite.%s", c->cmd);
>> +
>> + if (!git_config_get_string(key.buf, &v))
>> + c->enabled = git_config_bool(key.buf, v);
>> +
>> + if (!c->mode_from_env && !git_config_get_string("notes.rewritemode",
>> &v)) {
>> if (!v)
>> - return config_error_nonbool(k);
>> + config_error_nonbool("notes.rewritemode");
>
> There's a behavior change here. In the original code, the callback
> function would return -1, which would cause the program to die() if
> the config.c:die_on_error flag was set. The new code merely emits an
> error.
>
Is this change serious enough? Can I ignore it?
>> c->combine = parse_combine_notes_fn(v);
>
> Worse: Though you correctly emit an error when 'v' is NULL, you then
> (incorrectly) invoke parse_combine_notes_fn() with that NULL value,
> which will result in a crash.
>
Noted.
>> - if (!c->combine) {
>> + if (!c->combine)
>> error(_("Bad notes.rewriteMode value: '%s'"), v);
>> - return 1;
>> - }
>> - return 0;
>> - } else if (!c->refs_from_env && !strcmp(k, "notes.rewriteref")) {
>> + }
>> + if (!c->refs_from_env && !git_config_get_string("notes.rewriteref",
>> &v)) {
>> /* note that a refs/ prefix is implied in the
>> * underlying for_each_glob_ref */
>> if (starts_with(v, "refs/notes/"))
>> @@ -91,10 +92,8 @@ static int notes_rewrite_config(const char *k, const char
>> *v, void *cb)
>> else
>> warning(_("Refusing to rewrite notes in %s"
>> " (outside of refs/notes/)"), v);
>> - return 0;
>> }
>> -
>> - return 0;
>> + strbuf_release(&key);
>
> It would be better to release the strbuf immediately after its final
> use rather than waiting until the end of function. Not only does that
> reduce cognitive load on people reading the code, but it also reduces
> likelihood of 'key' being leaked if some future programmer inserts an
> early 'return' into the function for some reason.
>
Noted. Thanks.
>> }
>>
>>
>> @@ -123,7 +122,7 @@ struct notes_rewrite_cfg
>> *init_copy_notes_for_rewrite(const char *cmd)
>> c->refs_from_env = 1;
>> string_list_add_refs_from_colon_sep(c->refs,
>> rewrite_refs_env);
>> }
>> - git_config(notes_rewrite_config, c);
>> + notes_rewrite_config(c);
>> if (!c->enabled || !c->refs->nr) {
>> string_list_clear(c->refs, 0);
>> free(c->refs);
>> --
>> 1.9.0.GIT
>
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC/PATCH] notes.c: replace git_config with git_config_get_string
On 6/25/2014 1:36 PM, Eric Sunshine wrote:
> On Mon, Jun 23, 2014 at 6:41 AM, Tanay Abhra wrote:
>> Use git_config_get_string instead of git_config to take advantage of
>> the config hash-table api which provides a cleaner control flow.
>>
>> Signed-off-by: Tanay Abhra
>> ---
>> notes.c | 20 ++--
>> 1 file changed, 6 insertions(+), 14 deletions(-)
>>
>> diff --git a/notes.c b/notes.c
>> index 5fe691d..fc92eec 100644
>> --- a/notes.c
>> +++ b/notes.c
>> @@ -961,19 +961,6 @@ void string_list_add_refs_from_colon_sep(struct
>> string_list *list,
>> free(globs_copy);
>> }
>>
>> -static int notes_display_config(const char *k, const char *v, void *cb)
>> -{
>> - int *load_refs = cb;
>> -
>> - if (*load_refs && !strcmp(k, "notes.displayref")) {
>> - if (!v)
>> - config_error_nonbool(k);
>> - string_list_add_refs_by_glob(&display_notes_refs, v);
>> - }
>> -
>> - return 0;
>> -}
>> -
>> const char *default_notes_ref(void)
>> {
>> const char *notes_ref = NULL;
>> @@ -1041,6 +1028,7 @@ struct notes_tree **load_notes_trees(struct
>> string_list *refs)
>> void init_display_notes(struct display_notes_opt *opt)
>> {
>> char *display_ref_env;
>> + const char *value;
>> int load_config_refs = 0;
>> display_notes_refs.strdup_strings = 1;
>>
>> @@ -1058,7 +1046,11 @@ void init_display_notes(struct display_notes_opt *opt)
>> load_config_refs = 1;
>> }
>>
>> - git_config(notes_display_config, &load_config_refs);
>> + if (load_config_refs && !git_config_get_string("notes.displayref",
>> &value)) {
>> + if (!value)
>> + config_error_nonbool("notes.displayref");
>> + string_list_add_refs_by_glob(&display_notes_refs, value);
>
> Although you correctly diagnose a NULL 'value', you then invoke
> string_list_add_refs_by_glob() with that NULL, which will result in a
> crash.
>
> This is not a new error. It dates back to 894a9d33 (Support showing
> notes from more than one notes tree; 2010-03-12), but your rewrite
> should not retain the brokenness. Whether you fix it in this patch or
> a lead-in fix-up patch, the fix deserves mention in the commit
> message.
Done. Thanks.
>> + }
>>
>> if (opt) {
>> struct string_list_item *item;
>> --
>> 1.9.0.GIT
>
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC/PATCH V2] alias.c: replace git_config with git_config_get_string
On 6/25/2014 7:42 AM, Eric Sunshine wrote:
> On Mon, Jun 23, 2014 at 6:41 AM, Tanay Abhra wrote:
>> Use git_config_get_string instead of git_config to take advantage of
>> the config hash-table api which provides a cleaner control flow.
>>
>> Signed-off-by: Tanay Abhra
>> ---
>> alias.c | 28 ++--
>> 1 file changed, 10 insertions(+), 18 deletions(-)
>>
>> diff --git a/alias.c b/alias.c
>> index 5efc3d6..0fe32bc 100644
>> --- a/alias.c
>> +++ b/alias.c
>> @@ -1,25 +1,17 @@
>> #include "cache.h"
>>
>> -static const char *alias_key;
>> -static char *alias_val;
>> -
>> -static int alias_lookup_cb(const char *k, const char *v, void *cb)
>> -{
>> - if (starts_with(k, "alias.") && !strcmp(k + 6, alias_key)) {
>> - if (!v)
>> - return config_error_nonbool(k);
>> - alias_val = xstrdup(v);
>> - return 0;
>> - }
>> - return 0;
>> -}
>> -
>> char *alias_lookup(const char *alias)
>> {
>> - alias_key = alias;
>> - alias_val = NULL;
>> - git_config(alias_lookup_cb, NULL);
>> - return alias_val;
>> + const char *v;
>> + char *value;
>> + struct strbuf key = STRBUF_INIT;
>> + strbuf_addf(&key, "alias.%s", alias);
>> + git_config_get_string(key.buf, &v);
>> + if (!v)
>> + config_error_nonbool(key.buf);
>
> If 'v' is NULL, you correctly report an error, but then fall through
> and invoke xstrdup() with NULL, which invites undefined behavior [1].
>
> [1]: http://pubs.opengroup.org/onlinepubs/009695399/functions/strdup.html
>
>> + value = xstrdup(v);
>> + strbuf_release(&key);
>> + return value;
>
> You could release the strbuf earlier, which would allow you to 'return
> xstrdup(v)' and drop the 'value' variable. Perhaps you want something
> like this:
>
> const char *v;
> struct strbuf key = STRBUF_INIT;
> strbuf_addf(&key, "alias.%s", alias);
> git_config_get_string(key.buf, &v);
> if (v)
> config_error_nonbool(key.buf);
> strbuf_release(&key);
> return v ? xstrdup(v) : NULL;
>
Done. Thanks.
>> }
>>
>> #define SPLIT_CMDLINE_BAD_ENDING 1
>> --
>> 1.9.0.GIT
>
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC/PATCH] pager.c: replace git_config with git_config_get_string
On 6/25/2014 9:29 AM, Eric Sunshine wrote:
> On Mon, Jun 23, 2014 at 6:41 AM, Tanay Abhra wrote:
>> Use git_config_get_string instead of git_config to take advantage of
>> the config hash-table api which provides a cleaner control flow.
>>
>> Signed-off-by: Tanay Abhra
>> ---
>> pager.c | 44 +++-
>> 1 file changed, 15 insertions(+), 29 deletions(-)
>>
>> diff --git a/pager.c b/pager.c
>> index 8b5cbc5..96abe6d 100644
>> --- a/pager.c
>> +++ b/pager.c
>> @@ -6,12 +6,6 @@
>> #define DEFAULT_PAGER "less"
>> #endif
>>
>> -struct pager_config {
>> - const char *cmd;
>> - int want;
>> - char *value;
>> -};
>> -
>> /*
>> * This is split up from the rest of git so that we can do
>> * something different on Windows.
>> @@ -155,30 +149,22 @@ int decimal_width(int number)
>> return width;
>> }
>>
>> -static int pager_command_config(const char *var, const char *value, void
>> *data)
>> -{
>> - struct pager_config *c = data;
>> - if (starts_with(var, "pager.") && !strcmp(var + 6, c->cmd)) {
>> - int b = git_config_maybe_bool(var, value);
>> - if (b >= 0)
>> - c->want = b;
>> - else {
>> - c->want = 1;
>> - c->value = xstrdup(value);
>> - }
>> - }
>> - return 0;
>> -}
>> -
>> /* returns 0 for "no pager", 1 for "use pager", and -1 for "not specified"
>> */
>> int check_pager_config(const char *cmd)
>> {
>> - struct pager_config c;
>> - c.cmd = cmd;
>> - c.want = -1;
>> - c.value = NULL;
>> - git_config(pager_command_config, &c);
>> - if (c.value)
>> - pager_program = c.value;
>> - return c.want;
>> + struct strbuf key = STRBUF_INIT;
>> + int want = -1;
>> + const char *value = NULL;
>> + strbuf_addf(&key, "pager.%s", cmd);
>> + if (!git_config_get_string(key.buf, &value)) {
>> + int b = git_config_maybe_bool(key.buf, value);
>> + if (b >= 0)
>> + want = b;
>> + else
>> + want = 1;
>> + }
>> + if (value)
>> + pager_program = value;
>
> Two issues:
>
> First, why is 'if(value)' standing by itself? Although this works, it
> seems to imply that 'value' might be able to become non-NULL by some
> mechanism other than the get_config_maybe_bool() call, which means
> that people reading this code have to spend extra time trying to
> understand the overall logic. If you follow the example of the
> original code, where 'value' is only ever set when 'b < 0', then it is
> obvious even to the most casual reader that 'pager_program' is
> assigned only for that one condition.
>
Noted.
> Second, don't you want to xstrdup(value) when assigning to
> 'pager_program'? If you don't, then 'pager_program' will become a
> dangling pointer when config_cache_free() is invoked.
>
Noted. Thanks.
>> + strbuf_release(&key);
>> + return want;
>> }
>> --
>> 1.9.0.GIT
>
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 3/3] test-config: add usage examples for non-callback query functions
Hi, I thought about adding a test*.sh file after sending the series. No worries, I will rectify it in the next patch. Also, I have read all your comments. Thanks for the review. Cheers, Tanay Abhra. On 6/25/2014 4:49 PM, Eric Sunshine wrote: > On Mon, Jun 23, 2014 at 6:11 AM, Tanay Abhra wrote: >> Add different usage examples for 'git_config_get_string' and >> `git_config_get_string_multi`. They will serve as documentation >> on how to query for config values in a non-callback manner. > > This is a good start, but it's not fully what Matthieu was suggesting > when he said that you should prove to other developers, by way of > reproducible tests, that your changes work. What he meant, was that > you should write a test-config program which exposes (as a runnable > command) the new config C API you've added, and then write tests which > exercise that API and implementation exhaustively. > > For example, take a look at test-string-list.c and > t/t0063-string-list.sh. The C program does no checking itself. It > merely exposes the C API via command-line arguments, such as "split", > "filter", etc. The test script then employs that program to perform > the actual testing in a reproducible and (hopefully) exhaustive > fashion. Because t/t0063-string-list.sh is part of the test suite, the > string-list tests are run regularly by many developers. It's not just > something that someone might remember to run once in a while. > > Contrary to your commit message and the comment in the program itself, > the purpose of test-config is not to serve as documentation or to > provide examples of usage. (Real documentation is better suited for > those purposes.) Instead, test-config should exist in support of a > real test script in t/ which is run regularly. The new script you add > to t/ should exercise the exposed C API as exhaustively as possible. > This means checking each possible state: for instance, (1) when a key > is absent, (2) when a value is boolean (NULL), (3) one non-boolean > (non-NULL) value, (4) multiple values, etc. Moreover, it should check > expected success _and_ expected failure modes. Check not only that it > returns expected values, but that it fails when appropriate. > > More below. > -- 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
[BUG/RFC] cherry-pick adds newline to last line of file
Hi. If a file does not contain newline in the last line, and the file has changed somewhere in other branch, and the newline has not been not added in that change, when I cherry-pick the commit, the commit does contain the newline in the last line. This sometimes leads to conflict and in general looks unexpected. Such files are not uncommon nowadays and however bad it is, I think merging and cherry-picking should try to keep is as long as the newline is not added in some of the changes. Are there any option to preserve the absence of the closing newline? Or maybe the commands should preserve it unconditionally? -- Max -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v20 13/48] refs.c: make resolve_ref_unsafe set errno to something meaningful on error
Am 20.06.2014 16:42, schrieb Ronnie Sahlberg: > + errno = ELOOP; This fails on MinGW and MSVC < 2010. Perhaps add this to compat/mingw.h? #ifndef ELOOP #define ELOOP EMLINK #endif -- 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
GOOD DAY,
My name is Kong Hui from Hong Kong, I want you to be my partner in a business project. If Interested Contact me back via my email address. Thank you, Mr. Kong Hui. -- 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 (performance tracing)] test git-status performance
On Wed, Jun 25, 2014 at 5:56 AM, Karsten Blees wrote:
> void wt_status_collect(struct wt_status *s)
> {
> + uint64_t start = getnanotime();
> +
> wt_status_collect_changes_worktree(s);
>
> + trace_performance_since(start, "wt_status_collect_changes_worktree");
> + start = getnanotime();
> +
> if (s->is_initial)
> wt_status_collect_changes_initial(s);
> else
> wt_status_collect_changes_index(s);
> +
> + trace_performance_since(start, "wt_status_collect_changes_index");
> + start = getnanotime();
> +
> wt_status_collect_untracked(s);
> +
> + trace_performance_since(start, "wt_status_collect_untracked");
> }
Now that we have good perf measuring support, perhaps the next step is
remove gettimeofday() in advice_status_u_option related code in
wt_status_collect_untracked().
--
Duy
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Is it possible to list unpushed tags without accessing remote?
Hi, is it possible to know which tags are not yet pushed to a remote via a completely local command? (e.g. the list of unpushed _commits_ may be received by ‘git log ..’) I know it is possible to execute 'git ls-remote’ or 'git push --dry-run’, but both ask the remote server. I’m almost sure that the answer is “NO”, but want to receive a confirmation from Git gurus :) Thanks a lot! -- Kirill.-- 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] Fix: wrong offset for CET timezone
From: Alan Franzoni Signed-off-by: Alan Franzoni --- Documentation/date-formats.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Documentation/date-formats.txt b/Documentation/date-formats.txt index ccd1fc8..284308a 100644 --- a/Documentation/date-formats.txt +++ b/Documentation/date-formats.txt @@ -11,7 +11,7 @@ Git internal format:: It is ` `, where `` is the number of seconds since the UNIX epoch. `` is a positive or negative offset from UTC. - For example CET (which is 2 hours ahead UTC) is `+0200`. + For example CET (which is 1 hour ahead UTC) is `+0100`. RFC 2822:: The standard email format as described by RFC 2822, for example -- 2.0.0 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 4/8] add functions for memory-efficient bitmaps
On Thu, Jun 26, 2014 at 05:15:05AM +0200, Torsten Bögershausen wrote:
> > + */
> > +static inline int bitset_sizeof(int num_bits)
> > +{
> > + return (num_bits + CHAR_BIT - 1) / CHAR_BIT;
> > +}
> Just a general question about the usage of "int" here (and at other places):
> Is there a special reason for new code to allow num_bits to be negative ?
No. I usually choose "int" when the word size is not likely to matter
(i.e., we do not expect it to overflow a 32-bit integer, nor to have so
many that I need to be careful not to waste space).
Probably "unsigned int" would be a more descriptive choice.
It may also help the compiler optimize better. Assuming CHAR_BIT is 8
(i.e., most everywhere), we get:
(num_bits + 7) / 8
Presumably the compiler implements the division with a right-shift.
Marking num_bits as unsigned should let us do just a logical shift,
without worrying about the sign. And indeed, here are the signed and
unsigned versions produced by "gcc -S -O2" (for an equivalent
non-inlined function):
[signed]
leal14(%rdi), %edx
movl%edi, %eax
addl$7, %eax
cmovs %edx, %eax
sarl$3, %eax
ret
[unsigned]
leal7(%rdi), %eax
shrl$3, %eax
ret
Much simpler, though see below for practical considerations.
> To my knowledge all the size_t definitions these days are positive,
> because a size can not be negative.
size_t is perhaps a reasonable choice for the return value, given the name
"sizeof". But if you really care about using the whole range of bits there, you
need a data type for num_bits that is CHAR_BIT times larger.
> Should we use
> "unsigned" here ?
> or "unsigned int" ?
Yes, I think so. Both are the same to the compiler. I have a vague
recollection that we prefer one over the other, but grepping seems to
find many examples of each in our code.
I'm squashing in the patch below. I couldn't measure any speed
improvement. I'm guessing because the functions are all inlined, which
means we likely get away with calculating bitset_sizeof once outside of
our loop. I think the result is still more obvious to read, though.
-Peff
---
diff --git a/bitset.h b/bitset.h
index 5fbc956..268545b 100644
--- a/bitset.h
+++ b/bitset.h
@@ -45,7 +45,7 @@
*
* bits = xcalloc(1, bitset_sizeof(nr));
*/
-static inline int bitset_sizeof(int num_bits)
+static inline unsigned bitset_sizeof(unsigned num_bits)
{
return (num_bits + CHAR_BIT - 1) / CHAR_BIT;
}
@@ -54,7 +54,7 @@ static inline int bitset_sizeof(int num_bits)
* Set the bit at position "n". "n" is counted from zero, and must be
* smaller than the num_bits given to bitset_sizeof when allocating the bitset.
*/
-static inline void bitset_set(unsigned char *bits, int n)
+static inline void bitset_set(unsigned char *bits, unsigned n)
{
bits[n / CHAR_BIT] |= 1 << (n % CHAR_BIT);
}
@@ -62,7 +62,7 @@ static inline void bitset_set(unsigned char *bits, int n)
/*
* Return the bit at position "n" (see bitset_set for a description of "n").
*/
-static inline int bitset_get(unsigned char *bits, int n)
+static inline unsigned bitset_get(unsigned char *bits, unsigned n)
{
return !!(bits[n / CHAR_BIT] & (1 << (n % CHAR_BIT)));
}
@@ -75,9 +75,10 @@ static inline int bitset_get(unsigned char *bits, int n)
* "max" (we assume any bits beyond "max" up to the next CHAR_BIT boundary are
* zeroed padding).
*/
-static inline int bitset_equal(unsigned char *a, unsigned char *b, int max)
+static inline int bitset_equal(unsigned char *a, unsigned char *b,
+ unsigned max)
{
- int i;
+ unsigned i;
for (i = bitset_sizeof(max); i > 0; i--)
if (*a++ != *b++)
return 0;
@@ -89,9 +90,10 @@ static inline int bitset_equal(unsigned char *a, unsigned
char *b, int max)
*
* See bitset_equal for the definition of "max".
*/
-static inline void bitset_or(unsigned char *dst, const unsigned char *src, int
max)
+static inline void bitset_or(unsigned char *dst, const unsigned char *src,
+unsigned max)
{
- int i;
+ unsigned i;
for (i = bitset_sizeof(max); i > 0; i--)
*dst++ |= *src++;
}
@@ -101,9 +103,9 @@ static inline void bitset_or(unsigned char *dst, const
unsigned char *src, int m
*
* See bitset_equal for the definition of "max".
*/
-static inline int bitset_empty(const unsigned char *bits, int max)
+static inline int bitset_empty(const unsigned char *bits, unsigned max)
{
- int i;
+ unsigned i;
for (i = bitset_sizeof(max); i > 0; i--, bits++)
if (*bits)
return 0;
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 4/4] replace: add a --raw mode for --edit
On Wed, Jun 25, 2014 at 03:33:42PM -0700, Junio C Hamano wrote: > Jeff King writes: > > > One of the purposes of "git replace --edit" is to help a > > user repair objects which are malformed or corrupted. > > Usually we pretty-print trees with "ls-tree", which is much > > easier to work with than the raw binary data. However, some > > forms of corruption break the tree-walker, in which case our > > pretty-printing fails, rendering "--edit" useless for the > > user. > > > > This patch introduces a "--raw" option, which lets you edit > > the binary data in these instances. > > > > Signed-off-by: Jeff King > > --- > > Hm, this feels almost like inviting accidents to make it easy > and limiting the attempt to only one shot at the "transformation" > step. > > Will queue, but we can do the same with "cat-file $type >temp", do > some transformation on "temp", doubly make sure what is in "temp" is > sensible and then finally "hash-object -w -t $type temp". And it > makes me feel uneasy that the new feature seems to make it harder to > do the "doubly make sure" part. I do not think it is any worse than "--edit" is by itself. True, editing the binary contents is hard, but we do check that the format is sane when we read it back in (using the same checks that hash-object does). I think it would be nice to take that a step further and actually let hash-object (and "replace --edit") do the more rigorous fsck checks on created objects. I do still think even with those automated sanity checks that it makes sense to double-check the replacement manually. But I think that is one of the features of replace objects: you are not doing anything permanent, and can view the object in place. So not only can you examine the object by "git show $new_sha1", you can see it in place as "git log -p", etc. And easily back it out with "git replace -d" (or fix it up again with "git replace --edit") if need be. -Peff -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Is it possible to list unpushed tags without accessing remote?
On Thu, Jun 26, 2014 at 5:42 AM, Kirill Likhodedov wrote: > is it possible to know which tags are not yet pushed to a remote via a > completely local command? > > (e.g. the list of unpushed _commits_ may be received by ‘git log > ..’) > > I know it is possible to execute 'git ls-remote’ or 'git push --dry-run’, but > both ask the remote server. > I’m almost sure that the answer is “NO”, but want to receive a confirmation > from Git gurus :) No. The client doesn't track what tags the remote has. You may be able to guess by looking at `git log --decorate upstream..` and seeing which tags, if any show up. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 2/3] config: add hashtable for config parsing & retrieval
Ramsay Jones writes:
> Hmm, maybe. The "... take advantage of the new code" refers to the
> possibility (or otherwise) of re-using your work to update these
> "older API" functions to the new API style. (also, see Junio's response).
I agree that, while caching the usual config files is the most
important, allowing other users of the config code to use non-callback
functions would be nice too.
Not really for efficiency, but because I find it far more natural to ask
"what's the value of config key XYZ" to the code than to ask "can you
parse all config keys in the file, let me know, and I'll tell you later
which ones I'm interested in".
> [In order to do this, I would have expected to see one hash table
> for each file/blob, so the singleton object took me by surprise.]
The singleton could be fine as long as it is optional. We can have
an API looking like:
int git_config_get_string(const char *key, const char **value) {
return git_config_get_string_from(the singleton config cache,
key, value);
}
int git_config_get_string_from(struct hashmap *map, const char *key, const char
**value);
(or take a structure representing "a set of config files" instead of
directly the hashmap)
Now, the good news is that such API can be written later, without
breaking the API. So I'd say it's fine to keep the code as-is for now,
and make it more general later. No strong opinion, though.
--
Matthieu Moy
http://www-verimag.imag.fr/~moy/
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 2/3] config: add hashtable for config parsing & retrieval
Tanay Abhra writes:
> For the config_cache_free(), would this change be enough?
>
> +static void config_cache_free(void)
> +{
> + struct hashmap *config_cache;
> + struct config_cache_entry *entry;
> + struct hashmap_iter iter;
> + if (!hashmap_initialized)
> + return;
> + config_cache = get_config_cache();
That sounds better to me. And it justifies the different scopes: one can
ask whether the map is initialized from anywhere in the file, but can
only get the map after initialization, so it prevents mis-use of an
uninitialized pointer.
--
Matthieu Moy
http://www-verimag.imag.fr/~moy/
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC/PATCH V2] alias.c: replace git_config with git_config_get_string
Tanay Abhra writes: > the config hash-table api which provides a cleaner control flow. api -> API -- Matthieu Moy http://www-verimag.imag.fr/~moy/ -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 2/3] config: add hashtable for config parsing & retrieval
Tanay Abhra writes: > +Querying For Specific Variables > +--- > + > +For programs wanting to query for specific variables in a non-callback > +manner, the config API provides two functions `git_config_get_string` > +and `git_config_get_string_multi`.They both read values from an internal > +cache generated previously from reading the config files. > + > +`git_config_get_string` takes two parameters, > + > +- a key string in canonical flat form for which the corresponding value > + with the highest priority (i.e. value in the repo config will be > + preferred over value in user wide config for the same variable) will > + be retrieved. > + > +- a pointer to a string which will point to the retrieved value. > + > +`git_config_get_string` returns 0 for success, or -1 for no value found. > + > +`git_config_get_string_multi` returns a `string_list` containing all the > +values for the key passed as parameter, sorted in order of increasing > +priority (Note: NULL values are flagged as 1, check `util` for each > +'string_list_item` for flag value). I think you need to add something like: Both functions return pointers to values owned by the config cache. The caller need not free them, but should not modify them. -- Matthieu Moy http://www-verimag.imag.fr/~moy/ -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: files deleted with no record?
On Mon, Jun 23, 2014 at 9:15 PM, Jeremy Scott wrote:
> I just encountered a situation where a merge was made, with no
> apparent changes in files (ie no log), but the result was that some
> files were deleted.
>
> person A adds some files
> person B adds some files from the same point
You mean from the same point in history, right? Not files with the
same filename or path?
> person B commits and pushes.
> person A commits -- now merge is required
> a few more commits on top of person B's commit
I don't understand this step. Who adds a few more commits on top of B and why?
> person A merges - this removes the files that B added. There is no log
> of the files being deleted
This sounds like an "evil merge" (see man gitglossary, and google),
but it's not clear to me how you arrived here.
When I tried to reproduce your issue with this script, it did not
remove any files unexpectedly:
--->8---
#!/bin/sh
set -e
mkdir foo && cd foo && git init
echo foo > foo && git add foo && git commit -mfoo
git checkout -b PersonA master
echo bar > bar && git add bar
git commit -m"PersonA: bar"
git checkout -b PersonB master
echo baz > baz && git add baz
git commit -m"PersonB: baz"
echo foo-conflict >> foo && git add foo
git commit -m"PersonB: foo"
git checkout PersonA
echo foo-different >> foo && git add foo
git commit -m"PersonA: foo (conflicts with PersonB)"
git log --oneline --all --stat
if ! git merge PersonB ; then
git checkout PersonA foo
git commit --no-edit
fi
git log --oneline --graph --stat
--->8---
A sneaky issue with merges is that they do not have a clear way to
show patch information -- the diff between the commit and its ancestor
-- because they have multiple ancestors. You can show diffs for a
merge relative to each of its parents, but it is not usually done for
you automatically. This is why making changes in a merge which are
not actually required for the merge is bad ("evil").
> Clearly this is possible - was this a user error?
Probably, but we'd need to see how the user got there.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC/PATCH] imap-send.c: replace git_config with git_config_get_string
Tanay Abhra writes:
> + if (!git_config_get_string("imap.user", &value))
> + server.user = xstrdup(value);
> + if (!git_config_get_string("imap.pass", &value))
> + server.pass = xstrdup(value);
> + if (!git_config_get_string("imap.port", &value))
> + server.port = git_config_int("port", value);
> + if (!git_config_get_string("imap.tunnel", &value))
> + server.tunnel = xstrdup(value);
> + if (!git_config_get_string("imap.authmethod", &value))
> + server.auth_method = xstrdup(value);
Given this kind of systematic code, I find it very tempting to factor
this with a new helper function as
...
git_config_get_string_dup("imap.tunnel", &server.tunnel)
git_config_get_string_dup("imap.authmethod", &server.auth_method)
Is there any reason not to do so?
--
Matthieu Moy
http://www-verimag.imag.fr/~moy/
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Is it possible to list unpushed tags without accessing remote?
Shawn Pearce writes: > On Thu, Jun 26, 2014 at 5:42 AM, Kirill Likhodedov > wrote: >> is it possible to know which tags are not yet pushed to a remote via a >> completely local command? >> >> (e.g. the list of unpushed _commits_ may be received by ‘git log >> ..’) >> >> I know it is possible to execute 'git ls-remote’ or 'git push --dry-run’, >> but both ask the remote server. >> I’m almost sure that the answer is “NO”, but want to receive a confirmation >> from Git gurus :) > > No. The client doesn't track what tags the remote has. Not by default, but it is easy to configure your clone to fetch tags to a separate namespace. Andreas. -- Andreas Schwab, sch...@linux-m68k.org GPG Key fingerprint = 58CA 54C7 6D53 942B 1756 01D3 44D5 214B 8276 4ED5 "And now for something completely different." -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 2/3] config: add hashtable for config parsing & retrieval
Junio C Hamano writes:
> When the submodule script that uses "git config -f .gitmodules" is
> converted into C, if the updated config API is ready, it may be able
> to do something like these in a single program:
>
> const char *url;
> struct config_set *gm_config;
>
> /* read from $GIT_DIR/config */
> url = git_config_get_string("remote.origin.url");
>
> /*
> * allow us to read from .gitmodules; the parameters are
> * list of files that make up the configset, perhaps.
> */
> gm_config = git_configset_new(".gitmodules", NULL);
>
>
> if (!git_configset_get_bool(gm_config, "submodule.frotz.ignore")) {
> /* do a lot of stuff for the submodule */
> ...
> }
>
> /* when we are done with the configset */
> git_configset_clear(gm_config);
Isn't that a bit overkill? Why not just let the caller manage a hashmap
directly instead of a config_set? Your code could become
struct hashmap *gm_config;
config_cache_init(&gm_config);
config_cache_read_from_file(".gitmodule", gm_config);
/* possibly more calls to
config_cache_read_from_file("some-other-file", ...). */
and then
if (!git_configset_get_bool(gm_config, "submodule.frotz.ignore")) {
...
Perhaps a bit more complicated to use, but much simpler to implement.
Since there are very few callers, I'd say it's better to keep the
implementation simple.
--
Matthieu Moy
http://www-verimag.imag.fr/~moy/
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 4/4] diff: mark any file larger than core.bigfilethreshold binary
Nguyễn Thái Ngọc Duy writes:
> Too large files may lead to failure to allocate memory. If it happens
> here, it could impact quite a few commands that involve
> diff. Moreover, too large files are inefficient to compare anyway (and
> most likely non-text), so mark them binary and skip looking at their
> content.
> ...
> diff --git a/diff.c b/diff.c
> index a489540..7a977aa 100644
> --- a/diff.c
> +++ b/diff.c
> @@ -2185,8 +2185,8 @@ int diff_filespec_is_binary(struct diff_filespec *one)
> one->is_binary = one->driver->binary;
> else {
> if (!one->data && DIFF_FILE_VALID(one))
> - diff_populate_filespec(one, 0);
> - if (one->data)
> + diff_populate_filespec(one,
> DIFF_POPULATE_IS_BINARY);
> + if (one->is_binary == -1 && one->data)
> one->is_binary = buffer_is_binary(one->data,
> one->size);
> if (one->is_binary == -1)
The name is misleading and forced me to read it twice before I
realized that this is "populating the is-binary bit". It might make
it a bit better if you renamed it to DIFF_POPULATE_IS_BINARY_BIT or
perhaps DIFF_POPULATE_CHECK_BINARY or something. For consistency,
the other bit may want to be also renamed from SIZE_ONLY to either
(1) CHECK_SIZE_ONLY
(2) One bit for CHECK_SIZE, another for NO_CONTENTS, and optionally
make SIZE_ONLY the union of two
I do not have strong preference either way; the latter may be more
logical in that "not loading contents" and "check size" are sort of
orthogonal in that you can later choose to check, without loading
contents, only the binary-ness without checking size, but no calles
that passes a non-zero flag to the populate-filespec function will
want to slurp in the contents in practice, so in that sense we could
declare that the NO_CONENTS bit is implied.
But more importantly, would this patch actually help? For one
thing, this wouldn't (and shouldn't) help if the user wants --binary
diff out of us anyway, I suspect, but I wonder what the following
codepath in the builtin_diff() function would do:
...
} else if (!DIFF_OPT_TST(o, TEXT) &&
( (!textconv_one && diff_filespec_is_binary(one)) ||
(!textconv_two && diff_filespec_is_binary(two)) )) {
if (fill_mmfile(&mf1, one) < 0 || fill_mmfile(&mf2, two) < 0)
die("unable to read files to diff");
/* Quite common confusing case */
if (mf1.size == mf2.size &&
!memcmp(mf1.ptr, mf2.ptr, mf1.size)) {
if (must_show_header)
fprintf(o->file, "%s", header.buf);
goto free_ab_and_return;
}
fprintf(o->file, "%s", header.buf);
strbuf_reset(&header);
if (DIFF_OPT_TST(o, BINARY))
emit_binary_diff(o->file, &mf1, &mf2, line_prefix);
else
fprintf(o->file, "%sBinary files %s and %s differ\n",
line_prefix, lbl[0], lbl[1]);
o->found_changes = 1;
} else {
...
If we weren't told with --text/-a to force textual output, and
at least one of the sides is marked as binary (and this patch marks
a large blob as binary unless attributes says otherwise), we still
call fill_mmfile() on them to slurp the contents to be compared, no?
And before you get to the DIFF_OPT_TST(o, BINARY), you memcmp(3) to
check if the sides are identical, so...
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 2/4] fsck: do not die when not enough memory to examine a pack entry
Nguyễn Thái Ngọc Duy writes: > fsck is a tool that error() is more preferred than die(), but many "more preferred" without justifying why it is "more preferred" is not quite a justification, is it? Also, an object failing to load in-core is not a missing object, so if your aim is to let "fsck" diagnose a too-large-to-load object as missing and let it continue, I do not know if it is "more preferred" in the first place. Adding a "too large--cannot check" bin of objects may be needed for it to be useful. Also, we might need to give at the end "oh by the way, because we couldn't read some objects to even determine its type, the unreachable report from this fsck run is totally useless." The log message tries to justify that this may be a good thing for "fsck", but the patch actually tries to change the behaviour of all code paths that try to load an object in-core without considering the ramifications of such a change. I _think_ all callers should be prepared to receive NULL when we encounter a corrupt object (and otherwise we should fix them), but it is unclear how much audit of the callers (if any) was done to prepare this change. Not very excited X-<. > functions embed die() inside beyond fsck's control. > unpack_compressed_entry()'s using xmallocz is such a function, > triggered from verify_packfile() -> unpack_entry(). Make it use > xmallocz_gentle() instead. > > Noticed-by: Dale R. Worley > Signed-off-by: Nguyễn Thái Ngọc Duy > --- > sha1_file.c | 4 +++- > t/t1050-large.sh | 6 ++ > 2 files changed, 9 insertions(+), 1 deletion(-) > > diff --git a/sha1_file.c b/sha1_file.c > index 34d527f..eb69c78 100644 > --- a/sha1_file.c > +++ b/sha1_file.c > @@ -1925,7 +1925,9 @@ static void *unpack_compressed_entry(struct packed_git > *p, > git_zstream stream; > unsigned char *buffer, *in; > > - buffer = xmallocz(size); > + buffer = xmallocz_gentle(size); > + if (!buffer) > + return NULL; > memset(&stream, 0, sizeof(stream)); > stream.next_out = buffer; > stream.avail_out = size + 1; > diff --git a/t/t1050-large.sh b/t/t1050-large.sh > index aea4936..5642f84 100755 > --- a/t/t1050-large.sh > +++ b/t/t1050-large.sh > @@ -163,4 +163,10 @@ test_expect_success 'zip achiving, deflate' ' > git archive --format=zip HEAD >/dev/null > ' > > +test_expect_success 'fsck' ' > + test_must_fail git fsck 2>err && > + n=$(grep "error: attempting to allocate .* over limit" err | wc -l) && > + test "$n" -gt 1 > +' > + > test_done -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC/PATCH] pager.c: replace git_config with git_config_get_string
Am 25.06.2014 05:59, schrieb Eric Sunshine:
> On Mon, Jun 23, 2014 at 6:41 AM, Tanay Abhra wrote:
[...]
>> /* returns 0 for "no pager", 1 for "use pager", and -1 for "not specified"
>> */
>> int check_pager_config(const char *cmd)
>> {
>> - struct pager_config c;
>> - c.cmd = cmd;
>> - c.want = -1;
>> - c.value = NULL;
>> - git_config(pager_command_config, &c);
>> - if (c.value)
>> - pager_program = c.value;
>> - return c.want;
>> + struct strbuf key = STRBUF_INIT;
>> + int want = -1;
>> + const char *value = NULL;
>> + strbuf_addf(&key, "pager.%s", cmd);
>> + if (!git_config_get_string(key.buf, &value)) {
>> + int b = git_config_maybe_bool(key.buf, value);
>> + if (b >= 0)
>> + want = b;
>> + else
>> + want = 1;
>> + }
>> + if (value)
>> + pager_program = value;
[...]
>
> Second, don't you want to xstrdup(value) when assigning to
> 'pager_program'? If you don't, then 'pager_program' will become a
> dangling pointer when config_cache_free() is invoked.
>
I don't think that values from the global config cache should be xstrdup()ed.
After all, caching the values during the lifetime of the git process is the
entire point of the config cache, isn't it?
The only reason to call config_cache_free() is to load a _different_
configuration. In this case, however, you would also need to call the relevant
config functions again, leaking all xstrdup()ed strings.
If for some reason a config string is accessed after config_cache_free()
(which would be a bug), you won't notice if strings are xstrdup()ed (i.e. git
will continue to run with some invalid configuration). This is IMO much worse
than failing with segfault.
--
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 6/8] commit: provide a fast multi-tip contains function
Jeff King writes:
> I haven't quite convinced myself that the stale logic in the middle is
> right. The origin paint_down function checks "PARENT1 | PARENT2" to see
> if we found a merge base (even though PARENT2 may represent many tips).
> Here I check whether we have _any_ "left" parent flag and _any_ "right"
> parent flag. I'm not sure if I actually need to be finding the merge
> base of _all_ of the commits.
In the one-each-on-two-side case (i.e. the original algorithm), it
is "any commit we will encounter by digging further down from a
STALE one will all be reachable from a newer merge base (e.g. the
commit that caused it to be marked as STALE originally). It will
never be a useful merge base, so let's mark it as STALE. Even if a
future traversal that comes from sideways (i.e. not passing the
commit that caused it to be marked as STALE) reach this STALE one,
digging further from there won't give us anything new."
If you see a commit can be reached from L1 and R2, the only thing
you know is that its parents can also be reached from L1 and R2, but
it does not tell you if it is reachable from other tips, e.g. L2 or
R1. When a traversal reaches such a node from sideways, trying to
paint it with L2 for example, you do need to continue digging.
I think the traversal optimization based on the STALE bit is merely
a halfway optimization to cull obvious duplicates. See how
get_merge_bases_many() needs to clean up redundant ones in the
result of merge_bases_many(), the latter of which does use the STALE
bit to remove obvious duplicates, in order to make sure it won't
include a commit in the result that can be reached from another
commit in the result, without having to resort to the SLOP trick.
Because your end-goal is to tell if Ln is reachable from Rm (for
number of n's and m's), coming up with the independent/non-redundant
set of merge-bases is not necessary, I think. I suspect that you do
not need the STALE half-optimization in the first place.
The only time you can say "Ah, we've seen this one and no need to
dig further" is when you are propagating a colour C and the parent
in question is already painted in C (it is OK to be painted as
reachable from more tips), I would think, so shouldn't the loop be
more like, after painting each tip and placing it in the queue:
* Dequeue one, check the L/R bits in it and call that C
* Iterate over its parents P:
* check the L/R bits in P and call that Cp.
* If Cp is already a superset of C, there is no point putting P
to the queue to be processed.
* If Cp is not a superset of C, then update L/R bits in P to mark
it reachable from tips represented by C and put P to the queue.
until you ran out of queue?
> +void commit_contains(const struct commit_list *left,
> + const struct commit_list *right,
> + unsigned char *left_contains,
> + unsigned char *right_contains)
> +{
> + struct prio_queue queue = { compare_commits_by_commit_date };
> + struct bit_slab left_bits, right_bits, stale_bits;
> + int left_nr, right_nr;
> +
> + left_nr = init_contains_bits(left, &left_bits, &queue);
> + right_nr = init_contains_bits(right, &right_bits, &queue);
> + init_bit_slab(&stale_bits);
> +
> + while (queue_has_nonstale_bits(&queue, &stale_bits)) {
> + struct commit *commit = prio_queue_get(&queue);
> + struct commit_list *parents;
> + unsigned char *c_left, *c_right, *c_stale;
> +
> + c_left = bit_slab_at(&left_bits, commit);
> + c_right = bit_slab_at(&right_bits, commit);
> + c_stale = bit_slab_at(&stale_bits, commit);
> +
> + if (!bitset_empty(c_left, left_nr) &&
> + !bitset_empty(c_right, right_nr))
> + *c_stale = 1;
Hmph, is this one-char-a bit?
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 2/3] config: add hashtable for config parsing & retrieval
Matthieu Moy writes:
> Junio C Hamano writes:
>
>> When the submodule script that uses "git config -f .gitmodules" is
>> converted into C, if the updated config API is ready, it may be able
>> to do something like these in a single program:
>>
>> const char *url;
>> struct config_set *gm_config;
>>
>> /* read from $GIT_DIR/config */
>> url = git_config_get_string("remote.origin.url");
>>
>> /*
>> * allow us to read from .gitmodules; the parameters are
>> * list of files that make up the configset, perhaps.
>> */
>> gm_config = git_configset_new(".gitmodules", NULL);
>>
>>
>> if (!git_configset_get_bool(gm_config, "submodule.frotz.ignore")) {
>> /* do a lot of stuff for the submodule */
>> ...
>> }
>>
>> /* when we are done with the configset */
>> git_configset_clear(gm_config);
>
> Isn't that a bit overkill? Why not just let the caller manage a hashmap
> directly instead of a config_set?
Because I had an experience under my belt of a painful refactoring
of "the_index" which turned out to be not just a single array, I
simply suspect that the final data structure to represent a "set of
config-like things" will not be just a single hashmap, hence I do
prefer to have one layer of abstraction "struct config_set", which
would contain a hashmap and possibly more. Doesn't "is the hashmap
initialized" bit belong there, for example?
--
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: Is it possible to list unpushed tags without accessing remote?
Andreas Schwab writes: > Shawn Pearce writes: > >> On Thu, Jun 26, 2014 at 5:42 AM, Kirill Likhodedov >> wrote: >>> is it possible to know which tags are not yet pushed to a remote via a >>> completely local command? >>> >>> (e.g. the list of unpushed _commits_ may be received by ‘git log >>> ..’) >>> >>> I know it is possible to execute 'git ls-remote’ or 'git push --dry-run’, >>> but both ask the remote server. >>> I’m almost sure that the answer is “NO”, but want to receive a confirmation >>> from Git gurus :) >> >> No. The client doesn't track what tags the remote has. > > Not by default, but it is easy to configure your clone to fetch tags to > a separate namespace. But then in order to learn what tags the remote has, you need to talk to the remote and it won't be "complately a local" operation anymore, no? -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 2/3] config: add hashtable for config parsing & retrieval
Am 26.06.2014 21:00, schrieb Junio C Hamano:
> Matthieu Moy writes:
>
>> Junio C Hamano writes:
>>
>>> When the submodule script that uses "git config -f .gitmodules" is
>>> converted into C, if the updated config API is ready, it may be able
>>> to do something like these in a single program:
>>>
>>> const char *url;
>>> struct config_set *gm_config;
>>>
>>> /* read from $GIT_DIR/config */
>>> url = git_config_get_string("remote.origin.url");
>>>
>>> /*
>>> * allow us to read from .gitmodules; the parameters are
>>> * list of files that make up the configset, perhaps.
>>> */
>>> gm_config = git_configset_new(".gitmodules", NULL);
>>>
>>>
>>> if (!git_configset_get_bool(gm_config, "submodule.frotz.ignore")) {
>>> /* do a lot of stuff for the submodule */
>>> ...
>>> }
>>>
>>> /* when we are done with the configset */
>>> git_configset_clear(gm_config);
>>
>> Isn't that a bit overkill? Why not just let the caller manage a hashmap
>> directly instead of a config_set?
>
> Because I had an experience under my belt of a painful refactoring
> of "the_index" which turned out to be not just a single array, I
> simply suspect that the final data structure to represent a "set of
> config-like things" will not be just a single hashmap, hence I do
> prefer to have one layer of abstraction "struct config_set", which
> would contain a hashmap and possibly more. Doesn't "is the hashmap
> initialized" bit belong there, for example?
Would an additional
int hashmap_is_initialized(constr struct hashmap *map)
{
return !!map->table;
}
API help? (Note that hashmap_free() already does memset(0), so the usual notion
of "zero memory means unitialized" applies).
--
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 6/8] commit: provide a fast multi-tip contains function
Junio C Hamano writes:
> The only time you can say "Ah, we've seen this one and no need to
> dig further" is when you are propagating a colour C and the parent
> in question is already painted in C (it is OK to be painted as
> reachable from more tips), I would think, so shouldn't the loop be
> more like, after painting each tip and placing it in the queue:
>
> * Dequeue one, check the L/R bits in it and call that C
>
> * Iterate over its parents P:
>
>* check the L/R bits in P and call that Cp.
>
>* If Cp is already a superset of C, there is no point putting P
> to the queue to be processed.
>
>* If Cp is not a superset of C, then update L/R bits in P to mark
> it reachable from tips represented by C and put P to the queue.
>
> until you ran out of queue?
Actually that will cause you to dig down to the root, so it won't be
nice.
If you are trying to do "branch --with $commit", what you would want
is not exactly "paint-down-to-common(all branches, $commit)", but
something that paints down to $commit from all branches, with an
optimization that cuts off the traversal at a commit that is
reachable from $commit. If a traversal from branch B reached such a
commit that is reachable from $commit, you can stop the traversal
because propagating the bit for B further down to its parents will
not reach the $commit you are interested in.
So the termination condition for this a single Left (I'll use Left
for $commit and Right for "all branches") case is "if a commit is
already painted as reachable from $commit, do not propagate bits
further down".
What does it mean to look for "branch --with $commit1 $commit2"
(i.e. more than one in the Left set)? If we are trying to see which
branches reach _both_ of these commits, then replace the ablve with
"if a commit is already painted as reachable from both $commit{1,2},
then painting it with any branch bits is futile---its parents will
never reach either $commit1 nor $commit2", so the additional
termination condition will be "If left bits are full, then stop".
I do not know how you can optimize the traversal if you are trying
to see which branches reach at least one of these commits, though.
--
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: Is it possible to list unpushed tags without accessing remote?
On 26 Jun 2014, at 23:04 , Junio C Hamano wrote: > Andreas Schwab writes: > >> Not by default, but it is easy to configure your clone to fetch tags to >> a separate namespace. > > But then in order to learn what tags the remote has, you need to > talk to the remote and it won't be "complately a local" operation > anymore, no? If I understand the solution correctly, it looks like it is not needed, if I’m OK with knowing which tags the remote had during the last fetch. Just like with commits: 'git log origin/master..’ can give me incorrect results if e.g. commits were rebased on the remote site. But we usually ignore such possibility as improbable. -- 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 6/8] commit: provide a fast multi-tip contains function
Junio C Hamano writes:
> What does it mean to look for "branch --with $commit1 $commit2"
> (i.e. more than one in the Left set)? If we are trying to see which
> branches reach _both_ of these commits, then replace the ablve with
> "if a commit is already painted as reachable from both $commit{1,2},
> then painting it with any branch bits is futile---its parents will
> never reach either $commit1 nor $commit2", so the additional
> termination condition will be "If left bits are full, then stop".
>
> I do not know how you can optimize the traversal if you are trying
> to see which branches reach at least one of these commits, though.
By the way, don't we do 80% of this already in show-branch?
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: files deleted with no record?
From: "Phil Hord"
On Mon, Jun 23, 2014 at 9:15 PM, Jeremy Scott
wrote:
I just encountered a situation where a merge was made, with no
apparent changes in files (ie no log), but the result was that some
files were deleted.
person A adds some files
person B adds some files from the same point
You mean from the same point in history, right? Not files with the
same filename or path?
person B commits and pushes.
person A commits -- now merge is required
a few more commits on top of person B's commit
I don't understand this step. Who adds a few more commits on top of B
and why?
person A merges - this removes the files that B added. There is no
log
of the files being deleted
A similar issue, by reference, just came up on the [git-users] list. The
reference was:
1. http://randyfay.com/content/avoiding-git-disasters-gory-story
In that case the problem was a mixture of tools and misunderstandings.
It may not be the same as your case, but could give insights into what's
happening between different developers.
Which Tools are You, person A and Person B using, with --version?
This sounds like an "evil merge" (see man gitglossary, and google),
but it's not clear to me how you arrived here.
When I tried to reproduce your issue with this script, it did not
remove any files unexpectedly:
--->8---
#!/bin/sh
set -e
mkdir foo && cd foo && git init
echo foo > foo && git add foo && git commit -mfoo
git checkout -b PersonA master
echo bar > bar && git add bar
git commit -m"PersonA: bar"
git checkout -b PersonB master
echo baz > baz && git add baz
git commit -m"PersonB: baz"
echo foo-conflict >> foo && git add foo
git commit -m"PersonB: foo"
git checkout PersonA
echo foo-different >> foo && git add foo
git commit -m"PersonA: foo (conflicts with PersonB)"
git log --oneline --all --stat
if ! git merge PersonB ; then
git checkout PersonA foo
git commit --no-edit
fi
git log --oneline --graph --stat
--->8---
A sneaky issue with merges is that they do not have a clear way to
show patch information -- the diff between the commit and its ancestor
-- because they have multiple ancestors. You can show diffs for a
merge relative to each of its parents, but it is not usually done for
you automatically. This is why making changes in a merge which are
not actually required for the merge is bad ("evil").
Clearly this is possible - was this a user error?
Probably, but we'd need to see how the user got there.
--
Philip
--
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] gitk: catch mkdtemp errors
David Aguilar writes:
> 105b5d3fbb1c00bb0aeaf9d3e0fbe26a7b1993fc introduced a dependency
> on mkdtemp, which is not available on Windows.
>
> Use the original temporary directory behavior when mkdtemp fails.
> This makes the code use mkdtemp when available and gracefully
> fallback to the existing behavior when it is not available.
>
> Helped-by: Junio C Hamano
> Helped-by: brian m. carlson
> Signed-off-by: David Aguilar
> ---
Does this still need to be applied before I can pull from Paulus?
> gitk | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/gitk b/gitk
> index 41e5071..9237830 100755
> --- a/gitk
> +++ b/gitk
> @@ -3504,7 +3504,9 @@ proc gitknewtmpdir {} {
> set tmpdir $gitdir
> }
> set gitktmpformat [file join $tmpdir ".gitk-tmp.XX"]
> - set gitktmpdir [exec mktemp -d $gitktmpformat]
> + if {[catch {set gitktmpdir [exec mktemp -d $gitktmpformat]}]} {
> + set gitktmpdir [file join $gitdir [format ".gitk-tmp.%s" [pid]]]
> + }
> if {[catch {file mkdir $gitktmpdir} err]} {
> error_popup "[mc "Error creating temporary directory %s:"
> $gitktmpdir] $err"
> unset gitktmpdir
--
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] gitk: catch mkdtemp errors
David Aguilar writes:
> 105b5d3fbb1c00bb0aeaf9d3e0fbe26a7b1993fc introduced a dependency
> on mkdtemp, which is not available on Windows.
>
> Use the original temporary directory behavior when mkdtemp fails.
> This makes the code use mkdtemp when available and gracefully
> fallback to the existing behavior when it is not available.
>
> Helped-by: Junio C Hamano
> Helped-by: brian m. carlson
> Signed-off-by: David Aguilar
> ---
In the meantime, I've fetched from you and merged up to your
master~2 aka 17f9836c (gitk: Show staged submodules regardless of
ignore config, 2014-04-08).
Thanks.
> gitk | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/gitk b/gitk
> index 41e5071..9237830 100755
> --- a/gitk
> +++ b/gitk
> @@ -3504,7 +3504,9 @@ proc gitknewtmpdir {} {
> set tmpdir $gitdir
> }
> set gitktmpformat [file join $tmpdir ".gitk-tmp.XX"]
> - set gitktmpdir [exec mktemp -d $gitktmpformat]
> + if {[catch {set gitktmpdir [exec mktemp -d $gitktmpformat]}]} {
> + set gitktmpdir [file join $gitdir [format ".gitk-tmp.%s" [pid]]]
> + }
> if {[catch {file mkdir $gitktmpdir} err]} {
> error_popup "[mc "Error creating temporary directory %s:"
> $gitktmpdir] $err"
> unset gitktmpdir
--
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: files deleted with no record?
Hi. Thanks for getting back to me.
here is a screenshot from source tree to visualise the scenario:
https://drive.google.com/file/d/0B-Wn7DfHsuhyTEVkRHAzeGVZelpMWjFxZW1kbVBKVlNab3pR/edit?usp=sharing
I will attempt a script to reproduce this later today.
Thanks
On Fri, Jun 27, 2014 at 5:53 AM, Philip Oakley wrote:
> From: "Phil Hord"
>
>> On Mon, Jun 23, 2014 at 9:15 PM, Jeremy Scott
>> wrote:
>>>
>>> I just encountered a situation where a merge was made, with no
>>> apparent changes in files (ie no log), but the result was that some
>>> files were deleted.
>>>
>>> person A adds some files
>>> person B adds some files from the same point
>>
>>
>> You mean from the same point in history, right? Not files with the
>> same filename or path?
>>
>>> person B commits and pushes.
>>> person A commits -- now merge is required
>>> a few more commits on top of person B's commit
>>
>>
>> I don't understand this step. Who adds a few more commits on top of B and
>> why?
>>
>>> person A merges - this removes the files that B added. There is no log
>>> of the files being deleted
>
>
> A similar issue, by reference, just came up on the [git-users] list. The
> reference was:
> 1. http://randyfay.com/content/avoiding-git-disasters-gory-story
>
> In that case the problem was a mixture of tools and misunderstandings.
>
> It may not be the same as your case, but could give insights into what's
> happening between different developers.
>
> Which Tools are You, person A and Person B using, with --version?
>
>>
>> This sounds like an "evil merge" (see man gitglossary, and google),
>> but it's not clear to me how you arrived here.
>>
>> When I tried to reproduce your issue with this script, it did not
>> remove any files unexpectedly:
>> --->8---
>> #!/bin/sh
>>
>> set -e
>> mkdir foo && cd foo && git init
>> echo foo > foo && git add foo && git commit -mfoo
>>
>> git checkout -b PersonA master
>> echo bar > bar && git add bar
>> git commit -m"PersonA: bar"
>>
>> git checkout -b PersonB master
>> echo baz > baz && git add baz
>> git commit -m"PersonB: baz"
>>
>> echo foo-conflict >> foo && git add foo
>> git commit -m"PersonB: foo"
>>
>> git checkout PersonA
>> echo foo-different >> foo && git add foo
>> git commit -m"PersonA: foo (conflicts with PersonB)"
>>
>> git log --oneline --all --stat
>>
>> if ! git merge PersonB ; then
>> git checkout PersonA foo
>> git commit --no-edit
>> fi
>> git log --oneline --graph --stat
>> --->8---
>>
>> A sneaky issue with merges is that they do not have a clear way to
>> show patch information -- the diff between the commit and its ancestor
>> -- because they have multiple ancestors. You can show diffs for a
>> merge relative to each of its parents, but it is not usually done for
>> you automatically. This is why making changes in a merge which are
>> not actually required for the merge is bad ("evil").
>>
>>> Clearly this is possible - was this a user error?
>>
>>
>> Probably, but we'd need to see how the user got there.
>> --
>
> Philip
--
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: files deleted with no record?
we're all using source tree. I'm really interested to try and
reproduce this so I'll find some time today to do it.
Thanks again
On Fri, Jun 27, 2014 at 6:56 AM, Jeremy Scott wrote:
> Hi. Thanks for getting back to me.
>
> here is a screenshot from source tree to visualise the scenario:
>
> https://drive.google.com/file/d/0B-Wn7DfHsuhyTEVkRHAzeGVZelpMWjFxZW1kbVBKVlNab3pR/edit?usp=sharing
>
> I will attempt a script to reproduce this later today.
>
> Thanks
>
>
>
>
>
>
>
>
>
> On Fri, Jun 27, 2014 at 5:53 AM, Philip Oakley wrote:
>>
>> From: "Phil Hord"
>>
>>> On Mon, Jun 23, 2014 at 9:15 PM, Jeremy Scott
>>> wrote:
I just encountered a situation where a merge was made, with no
apparent changes in files (ie no log), but the result was that some
files were deleted.
person A adds some files
person B adds some files from the same point
>>>
>>>
>>> You mean from the same point in history, right? Not files with the
>>> same filename or path?
>>>
person B commits and pushes.
person A commits -- now merge is required
a few more commits on top of person B's commit
>>>
>>>
>>> I don't understand this step. Who adds a few more commits on top of B
>>> and why?
>>>
person A merges - this removes the files that B added. There is no log
of the files being deleted
>>
>>
>> A similar issue, by reference, just came up on the [git-users] list. The
>> reference was:
>> 1. http://randyfay.com/content/avoiding-git-disasters-gory-story
>>
>> In that case the problem was a mixture of tools and misunderstandings.
>>
>> It may not be the same as your case, but could give insights into what's
>> happening between different developers.
>>
>> Which Tools are You, person A and Person B using, with --version?
>>
>>>
>>> This sounds like an "evil merge" (see man gitglossary, and google),
>>> but it's not clear to me how you arrived here.
>>>
>>> When I tried to reproduce your issue with this script, it did not
>>> remove any files unexpectedly:
>>> --->8---
>>> #!/bin/sh
>>>
>>> set -e
>>> mkdir foo && cd foo && git init
>>> echo foo > foo && git add foo && git commit -mfoo
>>>
>>> git checkout -b PersonA master
>>> echo bar > bar && git add bar
>>> git commit -m"PersonA: bar"
>>>
>>> git checkout -b PersonB master
>>> echo baz > baz && git add baz
>>> git commit -m"PersonB: baz"
>>>
>>> echo foo-conflict >> foo && git add foo
>>> git commit -m"PersonB: foo"
>>>
>>> git checkout PersonA
>>> echo foo-different >> foo && git add foo
>>> git commit -m"PersonA: foo (conflicts with PersonB)"
>>>
>>> git log --oneline --all --stat
>>>
>>> if ! git merge PersonB ; then
>>> git checkout PersonA foo
>>> git commit --no-edit
>>> fi
>>> git log --oneline --graph --stat
>>> --->8---
>>>
>>> A sneaky issue with merges is that they do not have a clear way to
>>> show patch information -- the diff between the commit and its ancestor
>>> -- because they have multiple ancestors. You can show diffs for a
>>> merge relative to each of its parents, but it is not usually done for
>>> you automatically. This is why making changes in a merge which are
>>> not actually required for the merge is bad ("evil").
>>>
Clearly this is possible - was this a user error?
>>>
>>>
>>> Probably, but we'd need to see how the user got there.
>>> --
>>
>> Philip
>
>
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 2/3] config: add hashtable for config parsing & retrieval
Karsten Blees writes:
>> Because I had an experience under my belt of a painful refactoring
>> of "the_index" which turned out to be not just a single array, I
>> simply suspect that the final data structure to represent a "set of
>> config-like things" will not be just a single hashmap, hence I do
>> prefer to have one layer of abstraction "struct config_set", which
>> would contain a hashmap and possibly more. Doesn't "is the hashmap
>> initialized" bit belong there, for example?
>
> Would an additional
>
> int hashmap_is_initialized(constr struct hashmap *map)
> {
> return !!map->table;
> }
>
> API help? (Note that hashmap_free() already does memset(0), so the
> usual notion of "zero memory means unitialized" applies).
It may remove the need for the separate "hashmap_initialized" bit
that was implemented as a file-scope global in the patch.
I however am not convinced that it will be the _only_ thing other
than the hashmap we would need to use to keep track of the in-core
"set of config-like things", and usually a blanket statement "these
are the only thing we would ever need" tends not to hold for long,
so...
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
What's cooking in git.git (Jun 2014, #06; Thu, 26)
Here are the topics that have been cooking. Commits prefixed with
'-' are only in 'pu' (proposed updates) while commits prefixed with
'+' are in 'next'.
Fixes accumulated on the 'master' front made into 2.0.1. The topics
in flight continue to separate into two distinct layers (i.e.
stalled-and-need-to-be-rerolld vs sure-to-graduate-soon).
Four mingw series are still in limbo--are they in good enough shape
for Windows folks who wanted to upstream them?
You can find the changes described here in the integration branches
of the repositories listed at
http://git-blame.blogspot.com/p/git-public-repositories.html
--
[Graduated to "master"]
* ep/avoid-test-a-o (2014-06-19) 20 commits
(merged to 'next' on 2014-06-20 at c47322b)
+ git-submodule.sh: avoid "echo" path-like values
+ git-submodule.sh: avoid "test -a/-o "
+ t/test-lib-functions.sh: avoid "test -a/-o "
+ t/t9814-git-p4-rename.sh: avoid "test -a/-o "
+ t/t5538-push-shallow.sh: avoid "test -a/-o "
+ t/t5403-post-checkout-hook.sh: avoid "test -a/-o "
+ t/t5000-tar-tree.sh: avoid "test -a/-o "
+ t/t4102-apply-rename.sh: avoid "test -a/-o "
+ t/t0026-eol-config.sh: avoid "test -a/-o "
+ t/t0025-crlf-auto.sh: avoid "test -a/-o "
+ t/lib-httpd.sh: avoid "test -a/-o "
+ git-rebase--interactive.sh: avoid "test -a/-o "
+ git-mergetool.sh: avoid "test -a/-o "
+ git-bisect.sh: avoid "test -a/-o "
+ contrib/examples/git-resolve.sh: avoid "test -a/-o "
+ contrib/examples/git-repack.sh: avoid "test -a/-o "
+ contrib/examples/git-merge.sh: avoid "test -a/-o "
+ contrib/examples/git-commit.sh: avoid "test -a/-o "
+ contrib/examples/git-clone.sh: avoid "test -a/-o "
+ check_bindir: avoid "test -a/-o "
Update tests and scripts to avoid "test ... -a ...", which is often
more error-prone than "test ... && test ...".
Squashed misconversion fix-up into git-submodule.sh updates.
* fr/sequencer-fail-with-not-one-upon-no-ff (2014-06-09) 1 commit
(merged to 'next' on 2014-06-16 at 29734cc)
+ sequencer: signal failed ff as an aborted, not a conflicted merge
* jk/repack-pack-keep-objects (2014-06-10) 3 commits
(merged to 'next' on 2014-06-16 at 89716c9)
+ repack: s/write_bitmap/&s/ in code
+ repack: respect pack.writebitmaps
+ repack: do not accidentally pack kept objects by default
(this branch is used by jk/repack-pack-writebitmaps-config.)
Recent updates to "git repack" started to duplicate objects that
are in packfiles marked with .keep flag into the new packfile by
mistake.
* jk/repack-pack-writebitmaps-config (2014-06-12) 4 commits
(merged to 'next' on 2014-06-16 at 777005d)
+ t7700: drop explicit --no-pack-kept-objects from .keep test
+ repack: introduce repack.writeBitmaps config option
+ repack: simplify handling of --write-bitmap-index
+ pack-objects: stop respecting pack.writebitmaps
(this branch uses jk/repack-pack-keep-objects.)
* jm/dedup-name-compare (2014-06-20) 2 commits
+ cleanup duplicate name_compare() functions
+ name-hash.c: replace cache_name_compare() with memcmp(3)
* mc/doc-submodule-sync-recurse (2014-06-13) 1 commit
(merged to 'next' on 2014-06-20 at 04815e3)
+ submodule: document "sync --recursive"
* mc/git-p4-prepare-p4-only (2014-06-13) 1 commit
(merged to 'next' on 2014-06-16 at 3c05e19)
+ git-p4: fix submit in non --prepare-p4-only mode
* nd/init-restore-env (2014-06-10) 1 commit
(merged to 'next' on 2014-06-16 at ecbbfca)
+ git potty: restore environments after alias expansion
* pb/trim-trailing-spaces (2014-06-13) 1 commit
(merged to 'next' on 2014-06-20 at 6985153)
+ t0008: do not depend on 'echo' handling backslashes specially
* rs/blame-refactor (2014-06-13) 2 commits
(merged to 'next' on 2014-06-20 at ddaa722)
+ blame: simplify prepare_lines()
+ blame: factor out get_next_line()
* sp/complete-ext-alias (2014-06-13) 1 commit
(merged to 'next' on 2014-06-16 at 399679e)
+ completion: handle '!f() { ... }; f' and "!sh -c '...' -" aliases
* tb/unicode-7.0-display-width (2014-06-18) 1 commit
(merged to 'next' on 2014-06-20 at 111b246)
+ Update of unicode_width.h to Unicode Version 7.0
* ye/doc-http-proto (2014-06-16) 1 commit
(merged to 'next' on 2014-06-20 at 24f347d)
+ http-protocol.txt: Basic Auth is defined in RFC 2617, not RFC 2616
--
[New Topics]
* jc/fix-clone-single-starting-at-a-tag (2014-06-23) 1 commit
- builtin/clone.c: detect a clone starting at a tag correctly
Will merge to 'next'.
--
[Stalled]
* cc/replace-graft (2014-06-09) 5 commits
- DONTMERGE: wise to wait for peff's commit->buffer length series
- contrib: add convert-grafts-to-replace-refs.sh
- Documentation: replace: add --graft option
- replace: add test for --graft
- replace: add --graft option
"git replace" learned a "--graft" option to rewrite parents of a
commit.
Expecting a reroll on top of
Re: [PATCH] Fix: wrong offset for CET timezone
- Ursprungligt meddelande - > Från: "Alan Franzoni" > Till: git@vger.kernel.org > Kopia: "Alan Franzoni" > Skickat: torsdag, 26 jun 2014 15:53:32 > Ämne: [PATCH] Fix: wrong offset for CET timezone > > From: Alan Franzoni > > Signed-off-by: Alan Franzoni > --- > Documentation/date-formats.txt | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/Documentation/date-formats.txt b/Documentation/date-formats.txt > index ccd1fc8..284308a 100644 > --- a/Documentation/date-formats.txt > +++ b/Documentation/date-formats.txt > @@ -11,7 +11,7 @@ Git internal format:: > It is ` `, where ` timestamp>` is the number of seconds since the UNIX epoch. > `` is a positive or negative offset from UTC. > - For example CET (which is 2 hours ahead UTC) is `+0200`. > + For example CET (which is 1 hour ahead UTC) is `+0100`. 1 hour in winter and 2 in summer, although some standards seem to say that summer time is really called CEST, computers apply DST to CET in summer. $ TZ=UTC date Tor 26 Jun 2014 22:08:01 UTC $ TZ=CET date Fre 27 Jun 2014 00:08:05 CEST -- robin > RFC 2822:: > The standard email format as described by RFC 2822, for example > -- > 2.0.0 > > -- > 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: [RFC/PATCH] imap-send.c: replace git_config with git_config_get_string
Am 26.06.2014 18:50, schrieb Matthieu Moy:
> Tanay Abhra writes:
>
>> +if (!git_config_get_string("imap.user", &value))
>> +server.user = xstrdup(value);
>> +if (!git_config_get_string("imap.pass", &value))
>> +server.pass = xstrdup(value);
>> +if (!git_config_get_string("imap.port", &value))
>> +server.port = git_config_int("port", value);
>> +if (!git_config_get_string("imap.tunnel", &value))
>> +server.tunnel = xstrdup(value);
>> +if (!git_config_get_string("imap.authmethod", &value))
>> +server.auth_method = xstrdup(value);
>
> Given this kind of systematic code, I find it very tempting to factor
> this with a new helper function as
>
> ...
> git_config_get_string_dup("imap.tunnel", &server.tunnel)
> git_config_get_string_dup("imap.authmethod", &server.auth_method)
>
> Is there any reason not to do so?
>
With a pull-style API, you no longer need global variables for
everything, so IMO the helper functions should _return_ the values
rather than taking an output parameter.
E.g. with helper functions as suggested here [1] we could have:
if (git_config_get_bool("imap.preformattedhtml", 0))
wrap_in_html(&msg);
...rather than needing an extra variable:
int bool_value;
git_config_get_bool("imap.preformattedhtml", &bool_value);
if (bool_value)
wrap_in_html(&msg);
...and specify default values along with their respective keys:
server.ssl_verify = git_config_get_bool("imap.sslverify", 1);
server.port = git_config_get_int("imap.port", server.use_ssl ? 993 : 143);
...rather than ~1300 lines apart (yuck):
static struct imap_server_conf server = {
NULL, /* name */
NULL, /* tunnel */
NULL, /* host */
0, /* port */
NULL, /* user */
NULL, /* pass */
0, /* use_ssl */
1, /* ssl_verify */
0, /* use_html */
NULL, /* auth_method */
};
Regarding xstrdup(), I think this is a remnant from the callback
version, which _requires_ you to xstrdup() (the value parameter is
invalidated after returning from the callback).
Side note: with the current callback design, config variables may
get passed to the callback multiple times (last value wins), so
each xstrdup() in current 'git_*_config' functions actually
causes memory leaks (unless prefixed with 'free(my_config_var);').
[1]
http://git.661346.n2.nabble.com/PATCH-v3-0-3-git-config-cache-special-querying-api-utilizing-the-cache-tp7613911p7614050.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] gitk: catch mkdtemp errors
On Thu, Jun 26, 2014 at 01:42:04PM -0700, Junio C Hamano wrote: > David Aguilar writes: > > > 105b5d3fbb1c00bb0aeaf9d3e0fbe26a7b1993fc introduced a dependency > > on mkdtemp, which is not available on Windows. > > > > Use the original temporary directory behavior when mkdtemp fails. > > This makes the code use mkdtemp when available and gracefully > > fallback to the existing behavior when it is not available. > > > > Helped-by: Junio C Hamano > > Helped-by: brian m. carlson > > Signed-off-by: David Aguilar > > --- > > Does this still need to be applied before I can pull from Paulus? Yes, it would be nice to have this, especially for Windows users. -- David -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
How to populate index/worktree when recursive merge merges multiple common ancestors?
Imagine git does a recursive merge between A and B and finds multiple common ancestors X1,X2 for these commits. - Does git try to create an implicit/temporary common ancestor X3 by merging X1 and X2? - How should workingtree, index (stage1,2,3) look like if during that merge of common ancestors a conflict occurs? Will I see in stage2 and stage3 really see content of X1 and X2? - How is the end user supposed to fix this? Imaging merging X1 and X2 leads to conflicts solved by the end user leading to a implicit common ancestor X3. Then merging A and B with X3 as common base again conflicts occur. Ciao Chris -- 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: Tool/Scripts - For maintaining different branches on a repo
On Sat, Jun 21, 2014 at 4:00 AM, brian m. carlson wrote: > On Thu, Jun 19, 2014 at 04:18:22PM +0530, Jagan Teki wrote: >> Hi, >> >> I have a single repo with different kinds of branches say 4 branches. >> Developers will send a patches wrt to specific branch. > > I presume here that you're referring to emailed patches, or patches in > independent files, as opposed to just having branches with commits. Let me clear my requirement: I'm using Thunderbird, the tool will pick the patches with _defined_ subject prefix and apply the respective branches as I inputting. Please let me know if you still have any clarity. > >> Is there any opensource tool/script that does applying patches/maintaining >> the branches in repo w/o manual intervention? > > If you want something that works with patches specifically, TopGit might > do what you want. If what you're looking for is a tool that accepts > patches and automatically applies them, I'm not aware of one. It > shouldn't be terribly difficult to script, though. > > If you don't need to deal with patches and can instead deal with git > repositories, GitLab and Gitorious offer merge requests, which might > make life easier. I have heard that GitLab is less painful to set up. > > -- > brian m. carlson / brian with sandals: Houston, Texas, US > +1 832 623 2791 | http://www.crustytoothpaste.net/~bmc | My opinion only > OpenPGP: RSA v4 4096b: 88AC E9B2 9196 305B A994 7552 F1BA 225C 0223 B187 thanks! -- Jagan. -- 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
