Re: [PATCH v3 1/1] check-non-portable-shell.pl: Quoted `wc -l` is not portable
On Fri, Dec 22, 2017 at 01:07:59PM -0800, Junio C Hamano wrote: > tbo...@web.de writes: > > > > > Reviewed-by: Johannes Schindelin > > Signed-off-by: Torsten Bögershausen > > I'll flip these and add a helped-by to credit Eric. ... > Don't try to apply this patch to your tree yourself ;-) Thanks so much for cleaning up my mess.
Bring together merge and rebase
The big contention among git users is whether to rebase or to merge changes [2][3] while iterating. I used to firmly believe that merging was the way to go and rebase was harmful. More recently, I have worked in some environments where I saw rebase used very effectively while iterating on changes and I relaxed my stance a lot. Now, I'm on the fence. I appreciate the strengths and weaknesses of both approaches. I waffle between the two depending on the situation, the tools being used, and I guess, to some extent, my mood. I think what git needs is something brand new that brings the two together and has all of the advantages of both approaches. Let me explain what I've got in mind... I've been calling this proposal `git replay` or `git replace` but I'd like to hear other suggestions for what to name it. It works like rebase except with one very important difference. Instead of orphaning the original commit, it keeps a pointer to it in the commit just like a `parent` entry but calls it `replaces` instead to distinguish it from regular history. In the resulting commit history, following `parent` pointers shows exactly the same history as if the commit had been rebased. Meanwhile, the history of iterating on the change itself is available by following `replaces` pointers. The new commit replaces the old one but keeps it around to record how the change evolved. The git history now has two dimensions. The first shows a cleaned up history where fix ups and code review feedback have been rolled into the original changes and changes can possibly be ordered in a nice linear progression that is much easier to understand. The second drills into the history of a change. There is no loss and you don't change history in a way that will cause problems for others who have the older commits. Replay handles collaboration between multiple authors on a single change. This is difficult and prone to accidental loss when using rebase and it results in a complex history when done with merge. With replay, collaborators could merge while collaborating on a single change and a record of each one's contributions can be preserved. Attempting this level of collaboration caused me many headaches when I worked with the gerrit workflow (which in many ways, I like a lot). I blogged about this proposal earlier this year when I first thought of it [1]. I got busy and didn't think about it for a while. Now with a little time off of work, I've come back to revisit it. The blog entry has a few examples showing how it works and how the history will look in a few examples. Take a look. Various git commands will have to learn how to handle this kind of history. For example, things like fetch, push, gc, and others that move history around and clean out orphaned history should treat anything reachable through `replaces` pointers as precious. Log and related history commands may need new switches to traverse the history differently in different situations. Bisect is a interesting one. I tend to think that bisect should prefer the regular commit history but have the ability to drill into the change history if necessary. In my opinion, this proposal would bring together rebase and merge in a powerful way and could end the contention. Thanks for your consideration. Carl Baldwin [1] http://blog.episodicgenius.com/post/merge-or-rebase--neither/ [2] https://git-scm.com/book/en/v2/Git-Branching-Rebasing [3] http://changelog.complete.org/archives/586-rebase-considered-harmful
Re: Usability outrage as far as I am concerned
Hi Jacob, On Sat, Dec 23, 2017 at 12:05 AM, Jacob Keller wrote: > If you wish to update it later, you can mount hte usb stick, and then > just run git pull from inside the new subfolder. Note that you do > *not* run "git pull home_subfolder", as git pull expects the name of a > remote, which in this case is just origin (since the default remote > name you clone from is origin) >From git-fetch(8): The "remote" repository that is the source of a fetch or pull operation. This parameter can be either a URL (see the section GIT URLS below) or the name of a remote (see the section REMOTES below). You can run git fetch / git pull with a URL or a local path to a repository, not only origin etc. -- Mit freundlichen Grüßen, Anatolii Borodin
Re: [PATCH 2/3] move index_has_changes() from builtin/am.c to merge.c for reuse
On Fri, Dec 22, 2017 at 12:46 PM, Junio C Hamano wrote: > Elijah Newren writes: > >> On Thu, Dec 21, 2017 at 11:19 AM, Elijah Newren wrote: >>> index_has_changes() is a function we want to reuse outside of just am, >>> making it also available for merge-recursive and merge-ort. >>> >>> Signed-off-by: Elijah Newren >>> --- >> >> Note: These patches built on master, and merge cleanly with next and >> pu. However, this patch has a minor conflict with maint. If you'd >> prefer a version that applies on top of maint, let me know and I'll >> resubmit. > > I think I managed to create two topics, one that is with these three > patches (2/3 backported) on top of maint and the other one merges > the former on top of master. Please see if you found a mismerge > when I push the results out. I'm about to head out on a multi-day trip, so I might not get to this until the middle of next week. I'll try to take a look as soon as I can, though.
Re: [PATCH] Fix urlencode format string on signed char.
Hi Julien, On Sat, 23 Dec 2017, Julien Dusser wrote: > Thank you for review. I didn't find any other error. > Code in http.c:quote_ref_url() is almost the same but ch is a signed int, so > there's no issue. But that ch comes from a signed char *, so it actually *is* an issue: if you cast a signed char of value -1 to an int, it will still be -1. So we'll also need: -- snipsnap -- diff --git a/http.c b/http.c index 117ddae..ed8221f 100644 --- a/http.c +++ b/http.c @@ -1347,7 +1347,7 @@ void finish_all_active_slots(void) } /* Helpers for modifying and creating URLs */ -static inline int needs_quote(int ch) +static inline int needs_quote(unsigned char ch) { if (((ch >= 'A') && (ch <= 'Z')) || ((ch >= 'a') && (ch <= 'z')) @@ -1363,11 +1363,11 @@ static char *quote_ref_url(const char *base, const char *ref) { struct strbuf buf = STRBUF_INIT; const char *cp; - int ch; + unsigned char ch; end_url_with_slash(&buf, base); - for (cp = ref; (ch = *cp) != 0; cp++) + for (cp = ref; (ch = (unsigned char)*cp); cp++) if (needs_quote(ch)) strbuf_addf(&buf, "%%%02x", ch); else
Re: [PATCH] oidmap: ensure map is initialized
Hi Brandon, On Fri, 22 Dec 2017, Brandon Williams wrote: > Ensure that an oidmap is initialized before attempting to add, remove, > or retrieve an entry by simply performing the initialization step > before accessing the underlying hashmap. > > Signed-off-by: Brandon Williams > --- > oidmap.c | 11 +++ > 1 file changed, 11 insertions(+) > > diff --git a/oidmap.c b/oidmap.c > index 6db4fffcd..d9fb19ba6 100644 > --- a/oidmap.c > +++ b/oidmap.c > @@ -33,12 +33,19 @@ void oidmap_free(struct oidmap *map, int free_entries) > > void *oidmap_get(const struct oidmap *map, const struct object_id *key) > { > + if (!map->map.cmpfn) > + return NULL; > + This one is unnecessary: if we always _init() before we add, then hashmap_get_from_hash() will not have anything to compare, and therefore not call cmpfn. You could argue that it is only a teeny-tiny runtime cost, so it'd be better to be safe than to be sorry. However, it is a death by a thousand cuts. My colleagues and I try to shave off a few percent here and a few percent there, in the hope to get some performance improvements worth writing home about. And then patches like this one come in that simply add runtime without much in the way of performance considerations. And that makes me quite a bit sad. > return hashmap_get_from_hash(&map->map, hash(key), key); > } > > void *oidmap_remove(struct oidmap *map, const struct object_id *key) > { > struct hashmap_entry entry; > + > + if (!map->map.cmpfn) > + oidmap_init(map, 0); > + > hashmap_entry_init(&entry, hash(key)); > return hashmap_remove(&map->map, &entry, key); > } > @@ -46,6 +53,10 @@ void *oidmap_remove(struct oidmap *map, const struct > object_id *key) > void *oidmap_put(struct oidmap *map, void *entry) > { > struct oidmap_entry *to_put = entry; > + > + if (!map->map.cmpfn) > + oidmap_init(map, 0); > + > hashmap_entry_init(&to_put->internal_entry, hash(&to_put->oid)); > return hashmap_put(&map->map, to_put); > } "But it does not add a lot, it's only a couple of microseconds" Sure. But we could (and do) simply initialize the hashmaps once, and avoid having to spend unnecessary cycles for every single access. I *much* prefer my original patch that essentially does not change *any* code path. Everything stays the same, except that there is now a strong hint explaining why you need to call oidmap_init() manually instead of using the OIDMAP_INIT macro and then wonder why your code crashes. Ciao, Dscho
Re: [PATCH] oidmap.h: strongly discourage using OIDMAP_INIT directly
Hi Junio, On Fri, 22 Dec 2017, Junio C Hamano wrote: > Brandon Williams writes: > > >> -#define OIDMAP_INIT { { NULL } } > >> +/* > >> + * This macro initializes the data structure only incompletely, just > >> enough > >> + * to call oidmap_get() on an empty map. Use oidmap_init() instead. > >> + */ > >> +#define OIDMAP_INIT_INCOMPLETELY { { NULL } } > > > > This is one way of approaching the problem. Couldn't we also take the > > approach like we have with oidset and ensure that when oidmap_get() or > > _put() are called that if the oidmap isn't initialized, we simply do > > that then? > > Hmph. Can you show how the alternative code would look like? No, because I refuse to perform pointless work, in particular when I am already pretty booked with work. But you know how it would look like, right? The cmpfn() function would be exported via oidmap.h, and a HASHMAP_INIT(cmpfn) would be introduced in hashmap.h that would initialize everything zeroed out except for the cmpfn. But then you would review it and ask if there would be any use in adding cmp_cb_data to the signature of the HASHMAP_INIT() macro, and I would have to implement that, too. And then nobody would use it, and the macro would very likely get stale/incorrect. And then I would offer another patch reverting that change (because there is no user) and replace it with this here patch. As I said: pointless, Dscho
[PATCH 5/5] sequencer: do not invent whitespace when transforming OIDs
For commands that do not have an argument, there is no need to append a trailing space at the end of the line. Signed-off-by: Johannes Schindelin --- sequencer.c | 5 - 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/sequencer.c b/sequencer.c index 5632415ea2d..970842e3fcc 100644 --- a/sequencer.c +++ b/sequencer.c @@ -2584,7 +2584,10 @@ int transform_todos(unsigned flags) strbuf_addf(&buf, " %s", oid); } /* add all the rest */ - strbuf_addf(&buf, " %.*s\n", item->arg_len, item->arg); + if (!item->arg_len) + strbuf_addch(&buf, '\n'); + else + strbuf_addf(&buf, " %.*s\n", item->arg_len, item->arg); } i = write_message(buf.buf, buf.len, todo_file, 0); -- 2.15.1.windows.2
[PATCH 3/5] sequencer: remove superfluous conditional
In a conditional block that is only reached when handling a TODO_REWORD (as seen even from a 3-line context), there is absolutely no need to nest another block under the identical condition. Signed-off-by: Johannes Schindelin --- sequencer.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/sequencer.c b/sequencer.c index 8e6b6289be6..38266c3c228 100644 --- a/sequencer.c +++ b/sequencer.c @@ -1011,9 +1011,7 @@ static int do_pick_commit(enum todo_command command, struct commit *commit, opts); if (res || command != TODO_REWORD) goto leave; - flags |= EDIT_MSG | AMEND_MSG; - if (command == TODO_REWORD) - flags |= VERIFY_MSG; + flags |= EDIT_MSG | AMEND_MSG | VERIFY_MSG; msg_file = NULL; goto fast_forward_edit; } -- 2.15.1.windows.2
[PATCH 4/5] sequencer: report when noop has an argument
The noop command cannot accept any argument, but we never told the user about any bogus argument. Fix that. while at it, mention clearly when an argument is required but missing (for commands *other* than noop). Signed-off-by: Johannes Schindelin --- sequencer.c | 13 + 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/sequencer.c b/sequencer.c index 38266c3c228..5632415ea2d 100644 --- a/sequencer.c +++ b/sequencer.c @@ -1259,18 +1259,23 @@ static int parse_insn_line(struct todo_item *item, const char *bol, char *eol) if (i >= TODO_COMMENT) return -1; + /* Eat up extra spaces/ tabs before object name */ + padding = strspn(bol, " \t"); + bol += padding; + if (item->command == TODO_NOOP) { + if (bol != eol) + return error(_("%s does not accept arguments: '%s'"), +command_to_string(item->command), bol); item->commit = NULL; item->arg = bol; item->arg_len = eol - bol; return 0; } - /* Eat up extra spaces/ tabs before object name */ - padding = strspn(bol, " \t"); if (!padding) - return -1; - bol += padding; + return error(_("missing arguments for %s"), +command_to_string(item->command)); if (item->command == TODO_EXEC) { item->commit = NULL; -- 2.15.1.windows.2
[PATCH 2/5] sequencer: strip bogus LF at end of error messages
Signed-off-by: Johannes Schindelin --- sequencer.c | 8 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/sequencer.c b/sequencer.c index 115085d39ca..8e6b6289be6 100644 --- a/sequencer.c +++ b/sequencer.c @@ -491,7 +491,7 @@ static int is_index_unchanged(void) struct commit *head_commit; if (!resolve_ref_unsafe("HEAD", RESOLVE_REF_READING, &head_oid, NULL)) - return error(_("could not resolve HEAD commit\n")); + return error(_("could not resolve HEAD commit")); head_commit = lookup_commit(&head_oid); @@ -511,7 +511,7 @@ static int is_index_unchanged(void) if (!cache_tree_fully_valid(active_cache_tree)) if (cache_tree_update(&the_index, 0)) - return error(_("unable to update cache tree\n")); + return error(_("unable to update cache tree")); return !oidcmp(&active_cache_tree->oid, &head_commit->tree->object.oid); @@ -697,12 +697,12 @@ static int is_original_commit_empty(struct commit *commit) const struct object_id *ptree_oid; if (parse_commit(commit)) - return error(_("could not parse commit %s\n"), + return error(_("could not parse commit %s"), oid_to_hex(&commit->object.oid)); if (commit->parents) { struct commit *parent = commit->parents->item; if (parse_commit(parent)) - return error(_("could not parse parent commit %s\n"), + return error(_("could not parse parent commit %s"), oid_to_hex(&parent->object.oid)); ptree_oid = &parent->tree->object.oid; } else { -- 2.15.1.windows.2
[PATCH 0/5] A couple of sequencer cleanups
While working on patches to teach `git rebase -i` to recreate branch topology properly, i.e. replace the design of `--preserve-merges` by something much better, I stumbled across a couple of issues that I thought I should fix on the way. The patches are based on lb/rebase-i-short-command-names, mainly because the new `--recreate-merges` patches benefit from the patch "rebase -i: update functions to use a flags parameter", and I was too lazy to resolve the merge conflicts while rebasing the patch "sequencer: do not invent whitespace when transforming OIDs" to the current `master` branch. Oh, and by the way, the `--recreate-merges` feature already works. I used it to develop the patches themselves. I do not have time to pass one last time over them, so they'll have to wait for next year to see the Git mailing list. If anyone wants to have a look over them, play with them, or even wants to review the patches: https://github.com/git/git/compare/master...dscho:sequencer-shears Johannes Schindelin (5): rebase: do not continue when the todo list generation failed sequencer: strip bogus LF at end of error messages sequencer: remove superfluous conditional sequencer: report when noop has an argument sequencer: do not invent whitespace when transforming OIDs git-rebase--interactive.sh | 3 ++- sequencer.c| 30 ++ 2 files changed, 20 insertions(+), 13 deletions(-) base-commit: 1795993488bef1b48e4224db096e9d12df075db2 Based-On: lb/rebase-i-short-command-names at https://github.com/dscho/git Fetch-Base-Via: git fetch https://github.com/dscho/git lb/rebase-i-short-command-names Published-As: https://github.com/dscho/git/releases/tag/sequencer-cleanups-v1 Fetch-It-Via: git fetch https://github.com/dscho/git sequencer-cleanups-v1 -- 2.15.1.windows.2
[PATCH 1/5] rebase: do not continue when the todo list generation failed
This is a *really* long-standing bug. As a matter of fact, this bug has been with us from the very beginning of `rebase -i`: 1b1dce4bae7 (Teach rebase an interactive mode, 2007-06-25), where the output of `rev-list` was piped to `sed` (and any failure of the `rev-list` process would go completely undetected). Signed-off-by: Johannes Schindelin --- git-rebase--interactive.sh | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh index e3f5a0abf3c..b7f95672bd9 100644 --- a/git-rebase--interactive.sh +++ b/git-rebase--interactive.sh @@ -893,7 +893,8 @@ fi if test t != "$preserve_merges" then git rebase--helper --make-script ${keep_empty:+--keep-empty} \ - $revisions ${restrict_revision+^$restrict_revision} >"$todo" + $revisions ${restrict_revision+^$restrict_revision} >"$todo" || + die "$(gettext "Could not generate todo list")" else format=$(git config --get rebase.instructionFormat) # the 'rev-list .. | sed' requires %m to parse; the instruction requires %H to parse -- 2.15.1.windows.2
Re: [PATCH] Fix urlencode format string on signed char.
Thank you for review. I didn't find any other error. Code in http.c:quote_ref_url() is almost the same but ch is a signed int, so there's no issue. Le 22/12/2017 à 22:48, Junio C Hamano a écrit : Julien Dusser writes: Git credential fails with special char in password. remote: Invalid username or password. fatal: Authentication failed for File ~/.git-credential contains badly urlencoded characters %ffXX%ffYY instead of %XX%YY. Add a cast to an unsigned char to fix urlencode use of %02x on a char. Signed-off-by: Julien Dusser --- strbuf.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/strbuf.c b/strbuf.c index 323c49ceb..4d5a9ce55 100644 --- a/strbuf.c +++ b/strbuf.c @@ -658,7 +658,7 @@ static void strbuf_add_urlencode(struct strbuf *sb, const char *s, size_t len, (!reserved && is_rfc3986_reserved(ch))) strbuf_addch(sb, ch); else - strbuf_addf(sb, "%%%02x", ch); + strbuf_addf(sb, "%%%02x", (unsigned char)ch); } } The issue is not limited to credential but anywhere where we need to show a byte with hi-bit set, and it is obvious and straight-forward. I briefly wondered if the data type for the strings involved in the codepaths that reach this place should all be "uchar*" but it feels strange to have "unsigned char *username" etc., and the signeness matters only here, so the patch smells like the best one among other possibilities. Thanks.
[PATCH] oidmap: ensure map is initialized
Ensure that an oidmap is initialized before attempting to add, remove, or retrieve an entry by simply performing the initialization step before accessing the underlying hashmap. Signed-off-by: Brandon Williams --- oidmap.c | 11 +++ 1 file changed, 11 insertions(+) diff --git a/oidmap.c b/oidmap.c index 6db4fffcd..d9fb19ba6 100644 --- a/oidmap.c +++ b/oidmap.c @@ -33,12 +33,19 @@ void oidmap_free(struct oidmap *map, int free_entries) void *oidmap_get(const struct oidmap *map, const struct object_id *key) { + if (!map->map.cmpfn) + return NULL; + return hashmap_get_from_hash(&map->map, hash(key), key); } void *oidmap_remove(struct oidmap *map, const struct object_id *key) { struct hashmap_entry entry; + + if (!map->map.cmpfn) + oidmap_init(map, 0); + hashmap_entry_init(&entry, hash(key)); return hashmap_remove(&map->map, &entry, key); } @@ -46,6 +53,10 @@ void *oidmap_remove(struct oidmap *map, const struct object_id *key) void *oidmap_put(struct oidmap *map, void *entry) { struct oidmap_entry *to_put = entry; + + if (!map->map.cmpfn) + oidmap_init(map, 0); + hashmap_entry_init(&to_put->internal_entry, hash(&to_put->oid)); return hashmap_put(&map->map, to_put); } -- 2.15.1.620.gb9897f4670-goog
Re: [PATCHv5 7/7] builtin/describe.c: describe a blob
Stefan Beller writes: > On Tue, Dec 19, 2017 at 11:22 AM, Junio C Hamano wrote: >> I had to squash in the following to make 'pu' pass under >> gettext-poison build. Is this ready for 'next' otherwise? > > I saw that in pu, thanks for squashing. I should have spoken > up earlier confirming it. So is this ready for 'next' if it is squashed? >> With the "log --find-object" thing, it may be that this no longer is >> needed, but then again we haven't done anything with the other >> Jonathan's idea to unify the --find-object thing into the --pickaxe >> framework. > > I'll look into that after I finish looking at a submodule related bug. Thanks. >> It seems that we tend to open and then abandon new interests without >> seeing them through completion, leaving too many loose ends untied. >> This has to stop. > > I'll try to find my balance again. Sorry, but this was not really about you, but was more about me. I probably should more aggressively drop a topic that stalled and also merge a topic that reached the point of diminishing returns to 'next'.
Re: Usability outrage as far as I am concerned
On Fri, Dec 22, 2017 at 7:57 AM, Cristian Achim wrote: >> Can you show the output of "git remote" > > # in usb_subfolder > $git remote > origin > $ > > #in home_subfolder > $git remote > $ > With the -v switch you can see where each remote points to (tho your home local repo has no remote which is fine). >> and also >> clearly explain with details the layout of what the folders are and >> what is or is not a repository? > > Take the following update into consideration and then reread my first > email hopefully with improved clarity: > > 'home_subfolder' is the path on disk inside my user account home > folder in the 'home' root folder to the initial repo from which I > meant to do a backup. > > 'usb_subfolder' is the path on disk in the 'media' root folder to the > initial empty folder into which I wanted to do the backup above that > points into a usb stick I mounted in the default Kubuntu KDE file > manager way of mounting usb stick folder hierarchies. > > Current situation is that 'git log' in both home_subfolder and > usb_subfolder show the same hash with only one branch in both. From > usb_subfolder 'git pull home_subfolder' is broken as in the original > message. Ok. So you have a repository inside your home directory which you wish to copy into the USB stick? So what steps did you take to setup the repository usb_subfolder initially? You're basically trying to create a backup copy of what's in home_subfolder into your USB stick? If you're confident that home_subfolder is accurate right now, (ie: you inspect its contents with git log, git status, and regaulr commands to check that everything is as you expect), here's what I would do: cd to your usb stick, then run git clone /path/to/home_subfolder this will create an initial clone. If you wish to update it later, you can mount hte usb stick, and then just run git pull from inside the new subfolder. Note that you do *not* run "git pull home_subfolder", as git pull expects the name of a remote, which in this case is just origin (since the default remote name you clone from is origin) I'm still not certain what state you got in, but I believe based on your commands that the home_subfolder is fine, and you somehow incorrectly setup the usb_subfolder. Thanks, Jake PS/Tangent: If you never need the checked out files on the USB disk, and only wish to keep history saved, then you can actually do "git clone --mirror " in order to make a bare copy which is a complete mirror of all refs in the original repository. Then you can update it with just git fetch, or git remote update. (tho, keep in mind this clone would not have any working tree, but merely a bare repository contents). (You can, ofcourse, recover the files by simply cloning to somewhere else, or adding a new work tree or similar.
Re: [PATCH v2 2/2] commit: add support for --fixup -m""
Ævar Arnfjörð Bjarmason writes: > I don't agree that git as a tool should be so opinionated. You can edit > these --fixup messages right now with --edit, and I do. That it doesn't > work with -m"" as it should is a longstanding UI wart. I think you missed the point. I was expressing my opinion, not an opinion of Git as a tool, that I think one of these two "use case" scenario was a bad way not to be encouraged. That is totally different from allowing --fixup and -m working together. That is a good thing that helps the other "good" use case.
Re: [PATCH v2 2/2] commit: add support for --fixup -m""
On Fri, Dec 22 2017, Junio C. Hamano jotted: > Ævar Arnfjörð Bjarmason writes: > >> Those options could also support combining with -m, but given what >> they do I can't think of a good use-case for doing that, so I have not >> made the more invasive change of splitting up the logic in commit.c to >> first act on those, and then on -m options. >> >> 1. d71b8ba7c9 ("commit: --fixup option for use with rebase >>--autosquash", 2010-11-02) > > To be fair, when "rebase --autosquash -i" is run (which is why you > would use --fixup in the first place), the log message of the fixup > one is used only for locating which one is to be corrected, and the > contents of the log message is discarded. So "given what it does", > I can't think of a good use-case for using --fixup and -m together, > either. So "nobody immediately thought of it when it was added" is > certainly not a reason to later make the combination possible. > > But I personally am moderately negative on one of these two imagined > use cases. > >> Add support for supplying the -m option with --fixup. Doing so has >> errored out ever since --fixup was introduced. Before this, the only >> way to amend the fixup message while committing was to use --edit and >> amend it in the editor. >> >> The use-case for this feature is one of: >> >> * Leaving a quick note to self when creating a --fixup commit when >>it's not self-evident why the commit should be squashed without a >>note into another one. > > This is probably OK. > >> * (Ab)using the --fixup feature to "fix up" commits that have already >>been pushed to a branch that doesn't allow non-fast-forwards, >>i.e. just noting "this should have been part of that other commit", >>and if the history ever got rewritten in the future the two should >>be combined. > > This has a smell of the tail wagging the dog. > > Perhaps your editor does not have a good integration with external > commands to allow you to insert a single-liner output from > > git show --date=short -s --pretty='format:%h ("%s", %ad)' "$1" Since this use-case is talking about pushing a fixup for an already pushed commit, this is the first thing I put in -m"..." to reduce ambiguity. > and that is what you are abusing --fixup for? > > It is simply bad practice to leave a log entry that begins with > !fixup marker that would confuse automated tools like "rebase -i" > machinery on a commit that you have no intention of squashing into > another, as it invites mistakes. "if the history ever got rewritten in the future the two should be combined" So it's still the intent to squash these, it's just not being done right now, and even if it never happens it'll be easy to glance at the relevant commits in log --oneline. > I do agree with the scenario where you would wish you could take > back an earlier mistake but you cannot. But the log for such a > follow-up fix should be written just like any other follow-up fix > commit, i.e. describe what was wrong and how the wrongness is > corrected with the follow-up change. What was wrong in "which > commit" is of course important part, but it is a relatively small > part. I don't agree that git as a tool should be so opinionated. You can edit these --fixup messages right now with --edit, and I do. That it doesn't work with -m"" as it should is a longstanding UI wart. Tools should be naturally composable without needless arbitrary limitations.
Re: [PATCH] Fix urlencode format string on signed char.
Julien Dusser writes: > Git credential fails with special char in password. > remote: Invalid username or password. > fatal: Authentication failed for > > File ~/.git-credential contains badly urlencoded characters > %ffXX%ffYY instead of %XX%YY. > > Add a cast to an unsigned char to fix urlencode use of %02x > on a char. > > Signed-off-by: Julien Dusser > --- > strbuf.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/strbuf.c b/strbuf.c > index 323c49ceb..4d5a9ce55 100644 > --- a/strbuf.c > +++ b/strbuf.c > @@ -658,7 +658,7 @@ static void strbuf_add_urlencode(struct strbuf *sb, const > char *s, size_t len, > (!reserved && is_rfc3986_reserved(ch))) > strbuf_addch(sb, ch); > else > - strbuf_addf(sb, "%%%02x", ch); > + strbuf_addf(sb, "%%%02x", (unsigned char)ch); > } > } The issue is not limited to credential but anywhere where we need to show a byte with hi-bit set, and it is obvious and straight-forward. I briefly wondered if the data type for the strings involved in the codepaths that reach this place should all be "uchar*" but it feels strange to have "unsigned char *username" etc., and the signeness matters only here, so the patch smells like the best one among other possibilities. Thanks.
Re: [PATCH] sequencer: assign only free()able strings to gpg_sign
"phillip.w...@talktalk.net" writes: >>Original Message >>From: johannes.schinde...@gmx.de >>Date: 22/12/2017 11:50 >>To: >>Cc: "Junio C Hamano", "Phillip Wood" w...@dunelm.org.uk>, "Kaartic Sivaraam" >>Subj: [PATCH] sequencer: assign only free()able strings to gpg_sign >> >>The gpg_sign member of the replay_opts structure is of type `char *`, >>meaning that the sequencer deems the string to which gpg_sign points > to >>be under its custody, i.e. it needs to be free()d by the sequencer. >> >>Therefore, let's only assign malloc()ed buffers to it. >> >>Reported-by: Kaartic Sivaraam >>Signed-off-by: Johannes Schindelin >>--- >> >> Phillip, if you want to squash these changes into your patches, >> I'd totally fine with that. >> > > Hi Johannes, thanks for putting this together, the patch it fixes is > already in next so I think it'd be best to leave this one separate. I > wonder if it would be worth adding another test, see below. Thanks, both. Let's queue this on top as a fix-up.
Re: [PATCH v2 2/2] commit: add support for --fixup -m""
Junio C Hamano writes: > ... So "nobody immediately thought of it when it was added" is > certainly not a reason to later make the combination possible. Oops, not a reason to later "_not_ to" make an update, of course.
Re: [PATCH v2 2/2] commit: add support for --fixup -m""
Ævar Arnfjörð Bjarmason writes: > Those options could also support combining with -m, but given what > they do I can't think of a good use-case for doing that, so I have not > made the more invasive change of splitting up the logic in commit.c to > first act on those, and then on -m options. > > 1. d71b8ba7c9 ("commit: --fixup option for use with rebase >--autosquash", 2010-11-02) To be fair, when "rebase --autosquash -i" is run (which is why you would use --fixup in the first place), the log message of the fixup one is used only for locating which one is to be corrected, and the contents of the log message is discarded. So "given what it does", I can't think of a good use-case for using --fixup and -m together, either. So "nobody immediately thought of it when it was added" is certainly not a reason to later make the combination possible. But I personally am moderately negative on one of these two imagined use cases. > Add support for supplying the -m option with --fixup. Doing so has > errored out ever since --fixup was introduced. Before this, the only > way to amend the fixup message while committing was to use --edit and > amend it in the editor. > > The use-case for this feature is one of: > > * Leaving a quick note to self when creating a --fixup commit when >it's not self-evident why the commit should be squashed without a >note into another one. This is probably OK. > * (Ab)using the --fixup feature to "fix up" commits that have already >been pushed to a branch that doesn't allow non-fast-forwards, >i.e. just noting "this should have been part of that other commit", >and if the history ever got rewritten in the future the two should >be combined. This has a smell of the tail wagging the dog. Perhaps your editor does not have a good integration with external commands to allow you to insert a single-liner output from git show --date=short -s --pretty='format:%h ("%s", %ad)' "$1" and that is what you are abusing --fixup for? It is simply bad practice to leave a log entry that begins with !fixup marker that would confuse automated tools like "rebase -i" machinery on a commit that you have no intention of squashing into another, as it invites mistakes. I do agree with the scenario where you would wish you could take back an earlier mistake but you cannot. But the log for such a follow-up fix should be written just like any other follow-up fix commit, i.e. describe what was wrong and how the wrongness is corrected with the follow-up change. What was wrong in "which commit" is of course important part, but it is a relatively small part.
Re: [PATCH v3 1/1] check-non-portable-shell.pl: Quoted `wc -l` is not portable
tbo...@web.de writes: > > Reviewed-by: Johannes Schindelin > Signed-off-by: Torsten Bögershausen I'll flip these and add a helped-by to credit Eric. check-non-portable-shell.pl: `wc -l` may have leading WS Test scripts count number of lines in an output and check it againt its expectation. fb3340a6 ("test-lib: introduce test_line_count to measure files", 2010-10-31) introduced a helper to show a failure in such a test in a more readable way than comparing `wc -l` output with a number. Besides, on some platforms, "$(wc -l @@ -21,6 +21,7 @@ while (<>) { > /^\s*declare\s+/ and err 'arrays/declare not portable'; > /^\s*[^#]\s*which\s/ and err 'which is not portable (please use type)'; > /\btest\s+[^=]*==/ and err '"test a == b" is not portable (please use > =)'; > + /\bwc -l.*"\s*=/ and err '`"$(wc -l)"` is not portable (please use > test_line_count)'; > /\bexport\s+[A-Za-z0-9_]*=/ and err '"export FOO=bar" is not portable > (please use FOO=bar && export FOO)'; > # this resets our $. for each file > close ARGV if eof; Thanks. The dq before \s*= is rather cute ;-)
Re: [PATCH] config.txt: Document behavior of backslashes in subsections
Dave Borowitz writes: > Unrecognized escape sequences are invalid in values: > > $ git config -f - --list < [foo] > bar = "\t\\\y\"\u" > EOF > fatal: bad config line 2 in standard input > > But in subsection names, the backslash is simply dropped if the > following character does not produce a recognized escape sequence: > > $ git config -f - --list < [foo "\t\\\y\"\u"] > bar = baz > EOF > foo.t\y"u.bar=baz > > Although it would be nice for subsection names and values to have > consistent behavior, changing the behavior for subsection names is a > nonstarter since it would cause existing, valid config files to > suddenly be interpreted differently. > > Signed-off-by: Dave Borowitz > --- > Documentation/config.txt | 12 +++- > 1 file changed, 7 insertions(+), 5 deletions(-) Thanks. > > diff --git a/Documentation/config.txt b/Documentation/config.txt > index b18c0f97fe..f772186c44 100644 > --- a/Documentation/config.txt > +++ b/Documentation/config.txt > @@ -41,11 +41,13 @@ in the section header, like in the example below: > > > Subsection names are case sensitive and can contain any characters except > -newline (doublequote `"` and backslash can be included by escaping them > -as `\"` and `\\`, respectively). Section headers cannot span multiple > -lines. Variables may belong directly to a section or to a given subsection. > -You can have `[section]` if you have `[section "subsection"]`, but you > -don't need to. > +newline and the null byte. Doublequote `"` and backslash can be included > +by escaping them as `\"` and `\\`, respectively. Backslashes preceding > +other characters are dropped when reading; for example, `\t` is read as > +`t` and `\0` is read as `0` Section headers cannot span multiple lines. > +Variables may belong directly to a section or to a given subsection. You > +can have `[section]` if you have `[section "subsection"]`, but you don't > +need to. > > There is also a deprecated `[section.subsection]` syntax. With this > syntax, the subsection name is converted to lower-case and is also
Re: [PATCH 2/3] move index_has_changes() from builtin/am.c to merge.c for reuse
Elijah Newren writes: > On Thu, Dec 21, 2017 at 11:19 AM, Elijah Newren wrote: >> index_has_changes() is a function we want to reuse outside of just am, >> making it also available for merge-recursive and merge-ort. >> >> Signed-off-by: Elijah Newren >> --- > > Note: These patches built on master, and merge cleanly with next and > pu. However, this patch has a minor conflict with maint. If you'd > prefer a version that applies on top of maint, let me know and I'll > resubmit. I think I managed to create two topics, one that is with these three patches (2/3 backported) on top of maint and the other one merges the former on top of master. Please see if you found a mismerge when I push the results out. Thanks.
Re: [PATCH v2 0/2] support -m"" combined with commit --fixup
On Fri, Dec 22 2017, Eric Sunshine jotted: > On Fri, Dec 22, 2017 at 11:00 AM, Ævar Arnfjörð Bjarmason > wrote: >> Here's a hopefully ready to apply v2 incorporating feedback from Eric >> (thanks!). A tbdiff with v1 follows below. >> >> Ævar Arnfjörð Bjarmason (2): >> commit doc: document that -c, -C, -F and --fixup with -m error >> commit: add support for --fixup -m"" > > Patch 2/2 doesn't seem to have made it to the list... Oops, here it comes. >> 2: bd78a211ed ! 2: 780de6e042 commit: add support for --fixup >> -m"" >> @@ -22,6 +22,21 @@ >> In such a case you might want to leave a small message, >> e.g. "forgot this part, which broke XYZ". >> >> +With this, --fixup -m"More" -m"Details" will result in a >> +commit message like: >> + >> +!fixup > >> + >> +More >> + >> +Details >> + >> +The reason the test being added here seems to squash "More" at the >> end >> +of the subject line of the commit being fixed up is because the test >> +code is using "%s%b" so the body immediately follows the subject, >> it's >> +not a bug in this code, and other tests t7500-commit.sh do the same >> +thing. > > Did you also intend to mention something about --edit still working > with -m? (Or do we assume that people will understand automatically > that it does?) I thought it was clear enough since it works with --fixup now and with everything else, and the commit message was already getting long enough...
[PATCH v2 2/2] commit: add support for --fixup -m""
Add support for supplying the -m option with --fixup. Doing so has errored out ever since --fixup was introduced. Before this, the only way to amend the fixup message while committing was to use --edit and amend it in the editor. The use-case for this feature is one of: * Leaving a quick note to self when creating a --fixup commit when it's not self-evident why the commit should be squashed without a note into another one. * (Ab)using the --fixup feature to "fix up" commits that have already been pushed to a branch that doesn't allow non-fast-forwards, i.e. just noting "this should have been part of that other commit", and if the history ever got rewritten in the future the two should be combined. In such a case you might want to leave a small message, e.g. "forgot this part, which broke XYZ". With this, --fixup -m"More" -m"Details" will result in a commit message like: !fixup > More Details The reason the test being added here seems to squash "More" at the end of the subject line of the commit being fixed up is because the test code is using "%s%b" so the body immediately follows the subject, it's not a bug in this code, and other tests t7500-commit.sh do the same thing. When the --fixup option was initially added the "Option -m cannot be combined" error was expanded from -c, -C and -F to also include --fixup[1] Those options could also support combining with -m, but given what they do I can't think of a good use-case for doing that, so I have not made the more invasive change of splitting up the logic in commit.c to first act on those, and then on -m options. 1. d71b8ba7c9 ("commit: --fixup option for use with rebase --autosquash", 2010-11-02) Helped-by: Eric Sunshine Signed-off-by: Ævar Arnfjörð Bjarmason --- Documentation/git-commit.txt | 3 +-- builtin/commit.c | 8 +--- t/t7500-commit.sh| 9 - 3 files changed, 14 insertions(+), 6 deletions(-) diff --git a/Documentation/git-commit.txt b/Documentation/git-commit.txt index 3fbb7352bc..f970a43422 100644 --- a/Documentation/git-commit.txt +++ b/Documentation/git-commit.txt @@ -145,8 +145,7 @@ OPTIONS If multiple `-m` options are given, their values are concatenated as separate paragraphs. + -The `-m` option is mutually exclusive with `-c`, `-C`, `-F`, and -`--fixup`. +The `-m` option is mutually exclusive with `-c`, `-C`, and `-F`. -t :: --template=:: diff --git a/builtin/commit.c b/builtin/commit.c index 8a87701414..4e68394391 100644 --- a/builtin/commit.c +++ b/builtin/commit.c @@ -701,7 +701,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix, } } - if (have_option_m) { + if (have_option_m && !fixup_message) { strbuf_addbuf(&sb, &message); hook_arg1 = "message"; } else if (logfile && !strcmp(logfile, "-")) { @@ -731,6 +731,8 @@ static int prepare_to_commit(const char *index_file, const char *prefix, ctx.output_encoding = get_commit_output_encoding(); format_commit_message(commit, "fixup! %s\n\n", &sb, &ctx); + if (have_option_m) + strbuf_addbuf(&sb, &message); hook_arg1 = "message"; } else if (!stat(git_path_merge_msg(), &statbuf)) { /* @@ -1197,8 +1199,8 @@ static int parse_and_validate_options(int argc, const char *argv[], f++; if (f > 1) die(_("Only one of -c/-C/-F/--fixup can be used.")); - if (have_option_m && f > 0) - die((_("Option -m cannot be combined with -c/-C/-F/--fixup."))); + if (have_option_m && (edit_message || use_message || logfile)) + die((_("Option -m cannot be combined with -c/-C/-F."))); if (f || have_option_m) template_file = NULL; if (edit_message) diff --git a/t/t7500-commit.sh b/t/t7500-commit.sh index 5739d3ed23..2d95778b74 100755 --- a/t/t7500-commit.sh +++ b/t/t7500-commit.sh @@ -272,6 +272,14 @@ test_expect_success 'commit --fixup provides correct one-line commit message' ' commit_msg_is "fixup! target message subject line" ' +test_expect_success 'commit --fixup -m"something" -m"extra"' ' + commit_for_rebase_autosquash_setup && + git commit --fixup HEAD~1 -m"something" -m"extra" && + commit_msg_is "fixup! target message subject linesomething + +extra" +' + test_expect_success 'commit --squash works with -F' ' commit_for_rebase_autosquash_setup && echo "log message from file" >msgfile && @@ -325,7 +333,6 @@ test_expect_success 'invalid message options when using --fixup' ' test_must_fail git commit --fixup HEAD~1 --squash HEAD~2 && test_must_fail git commit --fixup HEAD~1 -C HEAD~2 && test_must_fail git commit --fixup HEAD~1 -c HEAD~2 && - test_must_fail git commit --fixup HEA
Re: [PATCH 3/3] merge-recursive: Avoid incorporating uncommitted changes in a merge
Elijah Newren writes: > builtin/merge.c contains this important requirement for merge strategies: > /* >* At this point, we need a real merge. No matter what strategy >* we use, it would operate on the index, possibly affecting the >* working tree, and when resolved cleanly, have the desired >* tree in the index -- this means that the index must be in >* sync with the head commit. The strategies are responsible >* to ensure this. >*/ > > merge-recursive does not do this check directly, instead it relies on > unpack_trees() to do it. However, merge_trees() has a special check for > the merge branch exactly matching the merge base; when it detects that > situation, it returns early without calling unpack_trees(), because it > knows that the HEAD commit already has the correct result. Unfortunately, > it didn't check that the index matched HEAD, so after it returned, the > outer logic ended up creating a merge commit that included something > other than HEAD. Good. I actually was imagining that you would shoot for creating an empty commit and leaving a working tree and the index that are both dirty, but I do not think it is worth the effort. Besides, "you have to start from a clean index" is a much simpler rule to explain than with "unless the resulting tree is the same as HEAD", especially when that "unless" is highly unlikely to happen anyway. Thanks. > > Signed-off-by: Elijah Newren > --- > merge-recursive.c| 7 +++ > t/t6044-merge-unrelated-index-changes.sh | 2 +- > 2 files changed, 8 insertions(+), 1 deletion(-) > > diff --git a/merge-recursive.c b/merge-recursive.c > index 2ecf495cc2..780f81a8bd 100644 > --- a/merge-recursive.c > +++ b/merge-recursive.c > @@ -1952,6 +1952,13 @@ int merge_trees(struct merge_options *o, > } > > if (oid_eq(&common->object.oid, &merge->object.oid)) { > + struct strbuf sb = STRBUF_INIT; > + > + if (index_has_changes(&sb)) { > + err(o, _("Dirty index: cannot merge (dirty: %s)"), > + sb.buf); > + return 0; > + } > output(o, 0, _("Already up to date!")); > *result = head; > return 1; > diff --git a/t/t6044-merge-unrelated-index-changes.sh > b/t/t6044-merge-unrelated-index-changes.sh > index 5e472be92b..23b86fb977 100755 > --- a/t/t6044-merge-unrelated-index-changes.sh > +++ b/t/t6044-merge-unrelated-index-changes.sh > @@ -112,7 +112,7 @@ test_expect_success 'recursive' ' > test_must_fail git merge -s recursive C^0 > ' > > -test_expect_failure 'recursive, when merge branch matches merge base' ' > +test_expect_success 'recursive, when merge branch matches merge base' ' > git reset --hard && > git checkout B^0 &&
Re: [PATCH] oidmap.h: strongly discourage using OIDMAP_INIT directly
Brandon Williams writes: >> -#define OIDMAP_INIT { { NULL } } >> +/* >> + * This macro initializes the data structure only incompletely, just enough >> + * to call oidmap_get() on an empty map. Use oidmap_init() instead. >> + */ >> +#define OIDMAP_INIT_INCOMPLETELY { { NULL } } > > This is one way of approaching the problem. Couldn't we also take the > approach like we have with oidset and ensure that when oidmap_get() or > _put() are called that if the oidmap isn't initialized, we simply do > that then? Hmph. Can you show how the alternative code would look like?
Re: [PATCH] send-pack: use internal argv_array of struct child_process
Stefan Beller writes: >> + argv_array_push(&po.args, "pack-objects"); >> + argv_array_push(&po.args, "--all-progress-implied"); >> + argv_array_push(&po.args, "--revs"); >> + argv_array_push(&po.args, "--stdout"); > > (useless nit of the day, no need to resend:) > These four statements could be done as pushl(a,b,c, NULL); > but it would not differ in readability, so I guess either is fine. Yup. Shorter is not necessarily better. If we anticipate that we will need to update the set of options later (and I suspect we may be doing so soon, given what JeffH and JTan are working on recently), one option (or option-argument pair) per line is a more preferrable form from the maintenance point of view.
Re: [PATCH] t/helper/test-lazy-name-hash: fix compilation
Stefan Beller writes: >> Heh; I do not think there particularly is much difference between >> stricter flags and DEVELOPER flags, but I would rather not lose the >> removal of duplicated 'today' I did while I queued the previous one >> ;-) > > > I did not realize it was queued anywhere, please ignore then. I just did s/stricter/DEVELOPER/ on the one that have been queued. Thanks.
Re: [PATCH 4/4] submodule: submodule_move_head omits old argument in forced case
Stefan Beller writes: > When using hard reset or forced checkout with the option to recurse into > submodules, the submodules need to be reset, too. > > It turns out that we need to omit the duplicate old argument to read-tree > in all forced cases to omit the 2 way merge and use the more assertive > behavior of reading the specific new tree into the index and updating > the working tree. The phrase "the more assertive" made me imagine something like "reset --hard", which resurrect lost paths and also get rid of added paths. "reading the specific new tree into the index" smells more like "checkout $tree-ish $paths" that has an overlay semantics, that resurrects lost paths but does not get rid of added paths. Perhaps not just "rm sub1/file1" but also add a new file that is not in HEAD to ensure that it will be blown away when $command is run to ensure that we got the distinction between the two right? > > Signed-off-by: Stefan Beller > --- > submodule.c | 4 +++- > t/lib-submodule-update.sh | 11 +++ > 2 files changed, 14 insertions(+), 1 deletion(-) > > diff --git a/submodule.c b/submodule.c > index fa25888783..db0f7ac51e 100644 > --- a/submodule.c > +++ b/submodule.c > @@ -1653,7 +1653,9 @@ int submodule_move_head(const char *path, > else > argv_array_push(&cp.args, "-m"); > > - argv_array_push(&cp.args, old ? old : EMPTY_TREE_SHA1_HEX); > + if (!(flags & SUBMODULE_MOVE_HEAD_FORCE)) > + argv_array_push(&cp.args, old ? old : EMPTY_TREE_SHA1_HEX); > + > argv_array_push(&cp.args, new ? new : EMPTY_TREE_SHA1_HEX); > > if (run_command(&cp)) { > diff --git a/t/lib-submodule-update.sh b/t/lib-submodule-update.sh > index fb0173ea87..380ef4b4ae 100755 > --- a/t/lib-submodule-update.sh > +++ b/t/lib-submodule-update.sh > @@ -1015,4 +1015,15 @@ test_submodule_forced_switch_recursing_with_args () { > test_submodule_content sub1 origin/modify_sub1 > ) > ' > + > + test_expect_success "$command: changed submodule worktree is reset" ' > + prolog && > + reset_work_tree_to_interested add_sub1 && > + ( > + cd submodule_update && > + rm sub1/file1 && > + $command HEAD && > + test_path_is_file sub1/file1 > + ) > + ' > }
Re: [PATCH 3/4] unpack-trees: Fix same() for submodules
Stefan Beller writes: > The function same(a, b) is used to check if two entries a and b are the > same. As the index contains the staged files the same() function can be > used to check if files between a given revision and the index are the same. > > In case of submodules, the gitlink entry is showing up as not modified > despite potential changes inside the submodule. > > Fix the function to examine submodules by looking inside the submodule. > This patch alone doesn't affect any correctness garantuees, but in guarantees? I somehow misread this as orangutan ;-) > combination with the next patch this fixes the new test introduced > earlier in this series. > > This may be a slight performance regression as now we have to > inspect any submodule thouroughly. > > Signed-off-by: Stefan Beller > --- > unpack-trees.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/unpack-trees.c b/unpack-trees.c > index bf8b602901..4d839e8edb 100644 > --- a/unpack-trees.c > +++ b/unpack-trees.c > @@ -1427,6 +1427,8 @@ static int same(const struct cache_entry *a, const > struct cache_entry *b) > return 1; > if ((a->ce_flags | b->ce_flags) & CE_CONFLICTED) > return 0; > + if (S_ISGITLINK(b->ce_mode) && should_update_submodules()) > + return !oidcmp(&a->oid, &b->oid) && > !is_submodule_modified(b->name, 0); Isn't this is working at the wrong level? It is technically correct to say "the function is used to check if two entries a and be are the same", but that statement misses a more important aspect of the function. It is asked if these two things are the same and does not get involved in seeing if the working tree is dirty with respect to (one of) the entries it was passed. For example, a virtual merge in a recursive merge would not (and should not) care if local changes exist in the working tree, and this function does not get any hint if it is to look at the working tree to tell such a case and the outermost merge apart. Somebody has to be careful not to stomp on local changes in the outmost merge in a recursive merge, but I do not think this function is designed to be the place to do so. Isn't a submodule with "potential changes" the same way? > return a->ce_mode == b->ce_mode && > !oidcmp(&a->oid, &b->oid); > }
Re: [PATCH v2 0/2] support -m"" combined with commit --fixup
On Fri, Dec 22, 2017 at 11:00 AM, Ævar Arnfjörð Bjarmason wrote: > Here's a hopefully ready to apply v2 incorporating feedback from Eric > (thanks!). A tbdiff with v1 follows below. > > Ævar Arnfjörð Bjarmason (2): > commit doc: document that -c, -C, -F and --fixup with -m error > commit: add support for --fixup -m"" Patch 2/2 doesn't seem to have made it to the list... > 2: bd78a211ed ! 2: 780de6e042 commit: add support for --fixup > -m"" > @@ -22,6 +22,21 @@ > In such a case you might want to leave a small message, > e.g. "forgot this part, which broke XYZ". > > +With this, --fixup -m"More" -m"Details" will result in a > +commit message like: > + > +!fixup > > + > +More > + > +Details > + > +The reason the test being added here seems to squash "More" at the > end > +of the subject line of the commit being fixed up is because the test > +code is using "%s%b" so the body immediately follows the subject, > it's > +not a bug in this code, and other tests t7500-commit.sh do the same > +thing. Did you also intend to mention something about --edit still working with -m? (Or do we assume that people will understand automatically that it does?)
Re: [PATCH 2/5] t/lib-submodule-update.sh: Fix test ignoring ignored files in submodules
Stefan Beller writes: > It turns out that this buggy test passed due to the buggy implementation, > which will soon be corrected. Let's fix the test first. Please clarify "buggy test". Without the original discussion (I am imagining there is something that happened on the list recently), here is my guess: We tried to make sure that an ignored file is $distimmed by $gostak, but forgot to tell the exclude mechanism that the path 'sub1'ignored' we use for the test is actually ignored, so the fact that the file was not $distimmed when $gostak did its thing meant nothing---mark any path whose name is 'ignored' as ignored to really test the condition we want to test. but I do not exactly know what verb to replace $distim and what noun to replace $gostak above ;-) Also, wouldn't this expose a bug in the implementation if that is the case? Thanks. > > Signed-off-by: Stefan Beller > --- > t/lib-submodule-update.sh | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/t/lib-submodule-update.sh b/t/lib-submodule-update.sh > index d7699046f6..fb0173ea87 100755 > --- a/t/lib-submodule-update.sh > +++ b/t/lib-submodule-update.sh > @@ -885,6 +885,7 @@ test_submodule_switch_recursing_with_args () { > ( > cd submodule_update && > git branch -t replace_sub1_with_file > origin/replace_sub1_with_file && > + echo ignored >.git/modules/sub1/info/exclude && > : >sub1/ignored && > $command replace_sub1_with_file && > test_superproject_content origin/replace_sub1_with_file > &&
Re: [PATCH v6] Makefile: replace perl/Makefile.PL with simple make rules
Ævar Arnfjörð Bjarmason writes: > Signed-off-by: Ævar Arnfjörð Bjarmason > --- Thanks, but I thought the patch was already in 'next' for a week or more and it's time to refine incrementally. Here is the difference as I see between what we already have and this update, and a proposed summary. -- >8 -- From: Ævar Arnfjörð Bjarmason Subject: perl: avoid *.pmc and fix Error.pm further The previous round tried to use *.pmc files but it confused RPM dependency analysis on some distros. Install them as plain vanilla *.pm files instead. Also "local @_" construct did not properly work when goto &sub is used until recent versions of Perl. Avoid it (and we do not need to localize it here anyway). --- diff --git a/Makefile b/Makefile index ba6607b7e7..5c73cd208a 100644 --- a/Makefile +++ b/Makefile @@ -2274,14 +2274,14 @@ endif po/build/locale/%/LC_MESSAGES/git.mo: po/%.po $(QUIET_MSGFMT)mkdir -p $(dir $@) && $(MSGFMT) -o $@ $< -PMFILES := $(wildcard perl/*.pm perl/*/*.pm perl/*/*/*.pm perl/*/*/*/*.pm) -PMCFILES := $(patsubst perl/%.pm,perl/build/lib/%.pmc,$(PMFILES)) +LIB_PERL := $(wildcard perl/Git.pm perl/Git/*.pm perl/Git/*/*.pm perl/Git/*/*/*.pm) +LIB_PERL_GEN := $(patsubst perl/%.pm,perl/build/lib/%.pm,$(LIB_PERL)) ifndef NO_PERL -all:: $(PMCFILES) +all:: $(LIB_PERL_GEN) endif -perl/build/lib/%.pmc: perl/%.pm +perl/build/lib/%.pm: perl/%.pm $(QUIET_GEN)mkdir -p $(dir $@) && \ sed -e 's|@@LOCALEDIR@@|$(localedir_SQ)|g' < $< > $@ diff --git a/perl/Git/Error.pm b/perl/Git/Error.pm index 5874672150..09bbc97390 100644 --- a/perl/Git/Error.pm +++ b/perl/Git/Error.pm @@ -39,7 +39,7 @@ sub import { require Error; }; -local @_ = ($caller, @_); +unshift @_, $caller; goto &Error::import; }
Re: Fetching commit instead of ref
"Carlsson, Magnus" writes: > $ git fetch subrepo > 50f730db793e0733b159326c5a3e78fd48cedfec:refs/remote/subrepo/foo-commit > remote: Counting objects: 2311, done. > remote: Total 2311 (delta 0), reused 0 (delta 0), pack-reused 2311 > -- >>> Receiving objects: 100% (2311/2311), 703.64 KiB | 0 bytes/s, done. > Resolving deltas: 100% (1174/1174), done. > error: Server does not allow request for unadvertised object > 50f730db793e0733b159326c5a3e78fd48cedfec Unless that "fetch" is doing a recursive fetch from another repository (which causes the counting to be shown) or something silly, the above makes it look like that the server is broken. A quick test locally does not seem to show such a behaviour, but I do not enable pack bitmaps and I know github does at least for some repositories, so it is possible there is a problem somewhere in that codepath.
Re: [PATCH] sequencer: assign only free()able strings to gpg_sign
Johannes Schindelin writes: >> I thought the bug could be triggered when commit.gpgsign was true and >> it was not overriden on the commandline, is it worth adding a test for >> that? > > ... If we want to verify that the > gpg_sign is correctly allocated before it is free()d, then the test case I > added *already* covers it,... It depends on what we are testing, how we anticipate this code will be broken by others in the future and how we want to futureproof the code. We can say "already covers it" if we know the implementation (especially, the code calls free() when it replaces opts->gpg_sign always, so the other side you are choosing not to test will die the same way) and assume it won't be broken (i.e. the attitude is OK for whitebox testing). I'd think that this particular case it is sufficient to test just one; I doubt that adding another will increse the test load measurably, though.
Re: [PATCH v2 1/5] core.aheadbehind: add new config setting
Jonathan Nieder writes: >> Created core.aheadbehind config setting and core_ahead_behind >> global variable. This value defaults to true. >> >> This value will be used in the next few commits as the default value >> for the --ahead-behind parameter. >> >> Signed-off-by: Jeff Hostetler >> --- >> Documentation/config.txt | 8 >> cache.h | 1 + >> config.c | 5 + >> environment.c| 1 + >> 4 files changed, 15 insertions(+) > > Not a reason to reroll on its own, but this seems out of order: the > series is easier to explain and easier to merge down in stages if the > patch for --ahead-behind comes first, then the config setting. > > More generally, new commandline flags tend to be less controversial > than new config settings since they cannot affect a script by mistake, > and for that reason, they can go earlier in the series. > > As a bonus, that makes it possible to include tests. It's probably > worth adding a test or two for this new config setting. > > [...] >> diff --git a/Documentation/config.txt b/Documentation/config.txt >> index 9593bfa..c78d6be 100644 >> --- a/Documentation/config.txt >> +++ b/Documentation/config.txt >> @@ -895,6 +895,14 @@ core.abbrev:: >> abbreviated object names to stay unique for some time. >> The minimum length is 4. >> >> +core.aheadbehind:: >> +If true, tells commands like status and branch to print ahead and >> +behind counts for the branch relative to its upstream branch. >> +This computation may be very expensive when there is a great >> +distance between the two branches. If false, these commands >> +only print that the two branches refer to different commits. >> +Defaults to true. > > This doesn't seem like a particularly core feature to me. Should it be > e.g. status.aheadbehind (even though it also affects "git branch") or > even something like diff.aheadbehind? I'm not sure. FWIW, I do not think it is core at all, either; sorry for not anticipating that a wrong name will be picked without a proper guidance when I saw the "not limited to status" mentioned in the discussion, but I was sick and offline for a few days, so... > I also wonder if there's a way to achieve the same benefit without > having it be configurable. E.g. if a branch is way behind, couldn't > we terminate the walk early to get the same bounded cost per branch > without requiring configuration? Hmm, that is an interesting thought.
Re: [PATCH] t/helper/test-lazy-name-hash: fix compilation
> Heh; I do not think there particularly is much difference between > stricter flags and DEVELOPER flags, but I would rather not lose the > removal of duplicated 'today' I did while I queued the previous one > ;-) I did not realize it was queued anywhere, please ignore then.
Re: [PATCH] t/helper/test-lazy-name-hash: fix compilation
Stefan Beller writes: > I was compiling origin/master today with the DEVELOPER compiler flags > today and was greeted by > > t/helper/test-lazy-init-name-hash.c: In function ‘cmd_main’: > t/helper/test-lazy-init-name-hash.c:172:5: error: ‘nr_threads_used’ may be > used uninitialized in this function [-Werror=maybe-uninitialized] > printf("avg [size %8d] [single %f] %c [multi %f %d]\n", > ^~~ > nr, > ~~~ > (double)avg_single/10, > ~~ > (avg_single < avg_multi ? '<' : '>'), > ~ > (double)avg_multi/10, > ~ > nr_threads_used); > > t/helper/test-lazy-init-name-hash.c:115:6: note: ‘nr_threads_used’ was > declared here > int nr_threads_used; > ^~~ > > Fix this issue by assigning 0 to 'nr_threads_used'. > > Acked-by: Jonathan Tan > Signed-off-by: Jeff Hostetler > Signed-off-by: Stefan Beller > --- > > Slightly reworded the commit message. I'd really like this patch to be > included > such that I can compile git with the DEVELOPER_CFLAGS flags. Heh; I do not think there particularly is much difference between stricter flags and DEVELOPER flags, but I would rather not lose the removal of duplicated 'today' I did while I queued the previous one ;-) > > t/helper/test-lazy-init-name-hash.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/t/helper/test-lazy-init-name-hash.c > b/t/helper/test-lazy-init-name-hash.c > index 6368a89345..297fb01d61 100644 > --- a/t/helper/test-lazy-init-name-hash.c > +++ b/t/helper/test-lazy-init-name-hash.c > @@ -112,7 +112,7 @@ static void analyze_run(void) > { > uint64_t t1s, t1m, t2s, t2m; > int cache_nr_limit; > - int nr_threads_used; > + int nr_threads_used = 0; > int i; > int nr;
Re: [PATCH] send-pack: use internal argv_array of struct child_process
On Fri, Dec 22, 2017 at 12:14 AM, René Scharfe wrote: > Avoid a magic number of NULL placeholder values and a magic index by > constructing the command line for pack-objects using the embedded > argv_array of the child_process. The resulting code is shorter and > easier to extend. > > Signed-off-by: Rene Scharfe Reviewed-by: Stefan Beller > --- > send-pack.c | 28 +--- > 1 file changed, 9 insertions(+), 19 deletions(-) > > diff --git a/send-pack.c b/send-pack.c > index a8cc6b266e..2112d3b27a 100644 > --- a/send-pack.c > +++ b/send-pack.c > @@ -58,35 +58,25 @@ static int pack_objects(int fd, struct ref *refs, struct > oid_array *extra, struc > * the revision parameters to it via its stdin and > * let its stdout go back to the other end. > */ > - const char *argv[] = { > - "pack-objects", > - "--all-progress-implied", > - "--revs", > - "--stdout", > - NULL, > - NULL, > - NULL, > - NULL, > - NULL, > - NULL, > - }; > struct child_process po = CHILD_PROCESS_INIT; > FILE *po_in; > int i; > int rc; > > - i = 4; > + argv_array_push(&po.args, "pack-objects"); > + argv_array_push(&po.args, "--all-progress-implied"); > + argv_array_push(&po.args, "--revs"); > + argv_array_push(&po.args, "--stdout"); (useless nit of the day, no need to resend:) These four statements could be done as pushl(a,b,c, NULL); but it would not differ in readability, so I guess either is fine. Thanks, Stefan > if (args->use_thin_pack) > - argv[i++] = "--thin"; > + argv_array_push(&po.args, "--thin"); > if (args->use_ofs_delta) > - argv[i++] = "--delta-base-offset"; > + argv_array_push(&po.args, "--delta-base-offset"); > if (args->quiet || !args->progress) > - argv[i++] = "-q"; > + argv_array_push(&po.args, "-q"); > if (args->progress) > - argv[i++] = "--progress"; > + argv_array_push(&po.args, "--progress"); > if (is_repository_shallow()) > - argv[i++] = "--shallow"; > - po.argv = argv; > + argv_array_push(&po.args, "--shallow"); > po.in = -1; > po.out = args->stateless_rpc ? -1 : fd; > po.git_cmd = 1; > -- > 2.15.1
[PATCH] Fix urlencode format string on signed char.
Git credential fails with special char in password. remote: Invalid username or password. fatal: Authentication failed for File ~/.git-credential contains badly urlencoded characters %ffXX%ffYY instead of %XX%YY. Add a cast to an unsigned char to fix urlencode use of %02x on a char. Signed-off-by: Julien Dusser --- strbuf.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/strbuf.c b/strbuf.c index 323c49ceb..4d5a9ce55 100644 --- a/strbuf.c +++ b/strbuf.c @@ -658,7 +658,7 @@ static void strbuf_add_urlencode(struct strbuf *sb, const char *s, size_t len, (!reserved && is_rfc3986_reserved(ch))) strbuf_addch(sb, ch); else - strbuf_addf(sb, "%%%02x", ch); + strbuf_addf(sb, "%%%02x", (unsigned char)ch); } } -- 2.15.1
Re: [PATCH] sequencer: assign only free()able strings to gpg_sign
Hi Phillip, On Fri, 22 Dec 2017, phillip.w...@talktalk.net wrote: > I thought the bug could be triggered when commit.gpgsign was true and > it was not overriden on the commandline, is it worth adding a test for > that? Everybody working with extensive test suites seems to write/blog these days that you have to be careful to test meaningfully, i.e. you need to avoid making running the test suite so expensive that developers start to avoid running it. In that light, what do we want to test? If we want to verify that the gpg_sign is correctly allocated before it is free()d, then the test case I added *already* covers it, and another test case would only increase the runtime of the test suite (which, as I hinted above, I deem a bad thing). So I'm really in favor of keeping the tests concise. Ciao, Dscho
Re: [PATCH] oidmap.h: strongly discourage using OIDMAP_INIT directly
On 12/22, Johannes Schindelin wrote: > In Git's source code, we have this convention that quite a few data > structures can be initialized using a macro *_INIT while defining an > instance (instead of having to call a function to initialize the data > structure). You will see that idiom quite a bit, e.g. `struct strbuf buf > = STRBUF_INIT;` > > This works for oidsets, too: `struct oidset oids = OIDSET_INIT;` is > perfectly legal and you can use that data structure right away, without > having to call `oidset_init()` first. > > That pattern is violated by OIDMAP_INIT, though. The first call to > oidmap_put() or oidmap_get() will succeed, but by mistake rather than by > design: The underlying hashmap is not initialized correctly, as the > cmpfn function pointer still points to NULL, but since there are no > entries to be compared, cmpfn will not be called. Things break down, > though, as soon as there is even one entry. > > Rather than causing confusion, frustration and needless loss of time due > to pointless debugging, let's *rename* OIDMAP_INIT so that developers > who have gotten used to the pattern `struct xyz a = XYZ_INIT;` won't do > that with oidmaps. > > An alternative would be to introduce a HASHMAP_INIT(cmp_fn) and use that > in oidmap.h. However, there are *no* call sites in Git's source code > that would benefit from that macro, i.e. this would be premature > optimization (and cost a lot more time than this here trivial patch). > > Signed-off-by: Johannes Schindelin > --- > Published-As: > https://github.com/dscho/git/releases/tag/discourage-OIDMAP_INIT-v1 > Fetch-It-Via: git fetch https://github.com/dscho/git discourage-OIDMAP_INIT-v1 > oidmap.h | 6 +- > oidset.h | 7 ++- > 2 files changed, 11 insertions(+), 2 deletions(-) > > diff --git a/oidmap.h b/oidmap.h > index 18f54cde143..1a73c392b79 100644 > --- a/oidmap.h > +++ b/oidmap.h > @@ -21,7 +21,11 @@ struct oidmap { > struct hashmap map; > }; > > -#define OIDMAP_INIT { { NULL } } > +/* > + * This macro initializes the data structure only incompletely, just enough > + * to call oidmap_get() on an empty map. Use oidmap_init() instead. > + */ > +#define OIDMAP_INIT_INCOMPLETELY { { NULL } } This is one way of approaching the problem. Couldn't we also take the approach like we have with oidset and ensure that when oidmap_get() or _put() are called that if the oidmap isn't initialized, we simply do that then? > > /* > * Initializes an oidmap structure. > diff --git a/oidset.h b/oidset.h > index f4c9e0f9c04..a11d88edc1d 100644 > --- a/oidset.h > +++ b/oidset.h > @@ -22,7 +22,12 @@ struct oidset { > struct oidmap map; > }; > > -#define OIDSET_INIT { OIDMAP_INIT } > +/* > + * It is okay to initialize the map incompletely here because oidset_insert() > + * will call oidset_init() (which will call oidmap_init()), and > + * oidset_contains() works as intended even before oidset_init() was called. > + */ > +#define OIDSET_INIT { OIDMAP_INIT_INCOMPLETELY } > > /** > * Returns true iff `set` contains `oid`. > > base-commit: 936d1b989416a95f593bf81ccae8ac62cd83f279 > -- > 2.15.1.windows.2 -- Brandon Williams
[PATCH v2 1/2] commit doc: document that -c, -C, -F and --fixup with -m error
Document that providing any of -c, -C, -F and --fixup along with -m will result in an error. Some variant of this has been errored about explicitly since 0c091296c0 ("git-commit: log parameter updates.", 2005-08-08), but the documentation was never updated to reflect this. Wording-by: Eric Sunshine Signed-off-by: Ævar Arnfjörð Bjarmason --- Documentation/git-commit.txt | 3 +++ 1 file changed, 3 insertions(+) diff --git a/Documentation/git-commit.txt b/Documentation/git-commit.txt index 8c74a2ca03..3fbb7352bc 100644 --- a/Documentation/git-commit.txt +++ b/Documentation/git-commit.txt @@ -144,6 +144,9 @@ OPTIONS Use the given as the commit message. If multiple `-m` options are given, their values are concatenated as separate paragraphs. ++ +The `-m` option is mutually exclusive with `-c`, `-C`, `-F`, and +`--fixup`. -t :: --template=:: -- 2.15.1.424.g9478a66081
[PATCH v2 0/2] support -m"" combined with commit --fixup
Here's a hopefully ready to apply v2 incorporating feedback from Eric (thanks!). A tbdiff with v1 follows below. Ævar Arnfjörð Bjarmason (2): commit doc: document that -c, -C, -F and --fixup with -m error commit: add support for --fixup -m"" Documentation/git-commit.txt | 2 ++ builtin/commit.c | 8 +--- t/t7500-commit.sh| 9 - 3 files changed, 15 insertions(+), 4 deletions(-) 1: 7d5e2531ee ! 1: 82333992ec commit doc: document that -c, -C, -F and --fixup with -m error @@ -7,6 +7,7 @@ explicitly since 0c091296c0 ("git-commit: log parameter updates.", 2005-08-08), but the documentation was never updated to reflect this. +Wording-by: Eric Sunshine Signed-off-by: Ævar Arnfjörð Bjarmason diff --git a/Documentation/git-commit.txt b/Documentation/git-commit.txt @@ -17,8 +18,8 @@ If multiple `-m` options are given, their values are concatenated as separate paragraphs. ++ -+Combining the `-m` option and any of `-c`, `-C`, `-F` or `--fixup` -+will result in an error. ++The `-m` option is mutually exclusive with `-c`, `-C`, `-F`, and ++`--fixup`. -t :: --template=:: 2: bd78a211ed ! 2: 780de6e042 commit: add support for --fixup -m"" @@ -22,6 +22,21 @@ In such a case you might want to leave a small message, e.g. "forgot this part, which broke XYZ". +With this, --fixup -m"More" -m"Details" will result in a +commit message like: + +!fixup > + +More + +Details + +The reason the test being added here seems to squash "More" at the end +of the subject line of the commit being fixed up is because the test +code is using "%s%b" so the body immediately follows the subject, it's +not a bug in this code, and other tests t7500-commit.sh do the same +thing. + When the --fixup option was initially added the "Option -m cannot be combined" error was expanded from -c, -C and -F to also include --fixup[1] @@ -34,6 +49,7 @@ 1. d71b8ba7c9 ("commit: --fixup option for use with rebase --autosquash", 2010-11-02) +Helped-by: Eric Sunshine Signed-off-by: Ævar Arnfjörð Bjarmason diff --git a/Documentation/git-commit.txt b/Documentation/git-commit.txt @@ -43,10 +59,9 @@ If multiple `-m` options are given, their values are concatenated as separate paragraphs. + --Combining the `-m` option and any of `-c`, `-C`, `-F` or `--fixup` --will result in an error. -+Combining the `-m` option and any of `-c`, `-C` or `-F` will result in -+an error. +-The `-m` option is mutually exclusive with `-c`, `-C`, `-F`, and +-`--fixup`. ++The `-m` option is mutually exclusive with `-c`, `-C`, and `-F`. -t :: --template=:: -- 2.15.1.424.g9478a66081
Re: Usability outrage as far as I am concerned
> Can you show the output of "git remote" # in usb_subfolder $git remote origin $ #in home_subfolder $git remote $ > and also > clearly explain with details the layout of what the folders are and > what is or is not a repository? Take the following update into consideration and then reread my first email hopefully with improved clarity: 'home_subfolder' is the path on disk inside my user account home folder in the 'home' root folder to the initial repo from which I meant to do a backup. 'usb_subfolder' is the path on disk in the 'media' root folder to the initial empty folder into which I wanted to do the backup above that points into a usb stick I mounted in the default Kubuntu KDE file manager way of mounting usb stick folder hierarchies. Current situation is that 'git log' in both home_subfolder and usb_subfolder show the same hash with only one branch in both. From usb_subfolder 'git pull home_subfolder' is broken as in the original message.
[PATCH] status: add a failing test showing a core.untrackedCache bug
The untracked cache gets confused when a directory is swapped out for a symlink to another directory. Whatever files are inside the target of the symlink will be incorrectly shown as untracked. This issue does not happen if the symlink links to another file, only if it links to another directory. A stand-alone testcase for copying into a terminal: ( rm -rf /tmp/testrepo && git init /tmp/testrepo && cd /tmp/testrepo && mkdir x y && touch x/a y/b && git add x/a y/b && git commit -msnap && git rm -rf y && ln -s x y && git add y && git commit -msnap2 && git checkout HEAD~ && git status && git checkout master && sleep 1 && git status && git status ) This will incorrectly show y/a as an untracked file. Both the "git status" call right before "git checkout master" and the "sleep 1" after the "checkout master" are needed to reproduce this, presumably due to the untracked cache tracking on the basis of cached whole seconds from stat(2). When git gets into this state, a workaround to fix it is to issue a one-off: git -c core.untrackedCache=false status Signed-off-by: Ævar Arnfjörð Bjarmason --- t/t7063-status-untracked-cache.sh | 45 +++ 1 file changed, 45 insertions(+) diff --git a/t/t7063-status-untracked-cache.sh b/t/t7063-status-untracked-cache.sh index e5fb892f95..7cf1e2c091 100755 --- a/t/t7063-status-untracked-cache.sh +++ b/t/t7063-status-untracked-cache.sh @@ -22,6 +22,12 @@ avoid_racy() { sleep 1 } +status_is_clean() { + >../status.expect && + git status --porcelain >../status.actual && + test_cmp ../status.expect ../status.actual +} + test_lazy_prereq UNTRACKED_CACHE ' { git update-index --test-untracked-cache; ret=$?; } && test $ret -ne 1 @@ -683,4 +689,43 @@ test_expect_success 'untracked cache survives a commit' ' test_cmp ../before ../after ' +test_expect_success 'teardown worktree' ' +cd .. +' + +test_expect_success 'setup worktree for symlink test' ' + git init worktree-symlink && + cd worktree-symlink && + git config core.untrackedCache true && + mkdir one two && + touch one/file two/file && + git add one/file two/file && + git commit -m"first commit" && + git rm -rf one && + ln -s two one && + git add one && + git commit -m"second commit" +' + +test_expect_failure '"status" after symlink replacement should be clean with UC=true' ' + git checkout HEAD~ && + status_is_clean && + status_is_clean && + git checkout master && + avoid_racy && + status_is_clean && + status_is_clean +' + +test_expect_success '"status" after symlink replacement should be clean with UC=false' ' + git config core.untrackedCache false && + git checkout HEAD~ && + status_is_clean && + status_is_clean && + git checkout master && + avoid_racy && + status_is_clean && + status_is_clean +' + test_done -- 2.15.1.424.g9478a66081
Re: [PATCH] sequencer: assign only free()able strings to gpg_sign
>Original Message >From: johannes.schinde...@gmx.de >Date: 22/12/2017 11:50 >To: >Cc: "Junio C Hamano", "Phillip Wood", "Kaartic Sivaraam" >Subj: [PATCH] sequencer: assign only free()able strings to gpg_sign > >The gpg_sign member of the replay_opts structure is of type `char *`, >meaning that the sequencer deems the string to which gpg_sign points to >be under its custody, i.e. it needs to be free()d by the sequencer. > >Therefore, let's only assign malloc()ed buffers to it. > >Reported-by: Kaartic Sivaraam >Signed-off-by: Johannes Schindelin >--- > > Phillip, if you want to squash these changes into your patches, > I'd totally fine with that. > Hi Johannes, thanks for putting this together, the patch it fixes is already in next so I think it'd be best to leave this one separate. I wonder if it would be worth adding another test, see below. Best Wishes Phillip >Based-On: pw/sequencer-in-process-commit at https://github.com/dscho/git >Fetch-Base-Via: git fetch https://github.com/dscho/git pw/sequencer- in-process-commit >Published-As: >https://github.com/dscho/git/releases/tag/sequencer-owns-gpg-sign-v1 >Fetch-It-Via: git fetch https://github.com/dscho/git sequencer-owns- gpg-sign-v1 > sequencer.c | 2 +- > t/t3404-rebase-interactive.sh | 10 ++ > 2 files changed, 11 insertions(+), 1 deletion(-) > >diff --git a/sequencer.c b/sequencer.c >index 7051b20b762..1b2599668f5 100644 >--- a/sequencer.c >+++ b/sequencer.c >@@ -160,7 +160,7 @@ static int git_sequencer_config(const char *k, const char *v, void *cb) > } > > if (!strcmp(k, "commit.gpgsign")) { >- opts->gpg_sign = git_config_bool(k, v) ? "" : NULL; >+ opts->gpg_sign = git_config_bool(k, v) ? xstrdup("") : NULL; > return 0; > } > >diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase- interactive.sh >index 9ed0a244e6c..040ef1a4dbc 100755 >--- a/t/t3404-rebase-interactive.sh >+++ b/t/t3404-rebase-interactive.sh >@@ -1318,6 +1318,16 @@ test_expect_success 'editor saves as CR/LF' ' > > SQ="'" > test_expect_success 'rebase -i --gpg-sign=' ' >+ test_when_finished "test_might_fail git rebase --abort" && >+ set_fake_editor && >+ FAKE_LINES="edit 1" git rebase -i --gpg-sign="\"S I Gner\"" HEAD^ \ >+ >out 2>err && >+ test_i18ngrep "$SQ-S\"S I Gner\"$SQ" err >+' >+ >+test_expect_success 'rebase -i --gpg-sign= overrides commit. gpgSign' ' >+ test_when_finished "test_might_fail git rebase --abort" && >+ test_config commit.gpgsign true && > set_fake_editor && > FAKE_LINES="edit 1" git rebase -i --gpg-sign="\"S I Gner\"" HEAD^ \ > >out 2>err && I thought the bug could be triggered when commit.gpgsign was true and it was not overriden on the commandline, is it worth adding a test for that? >base-commit: 28d6daed4f119940ace31e523b3b272d3d153d04 >-- >2.15.1.windows.2 >
[PATCH v2] http: use internal argv_array of struct child_process
Avoid a strangely magic array size (it's slightly too big) and explicit index numbers by building the command line for index-pack using the embedded argv_array of the child_process. Add the flag -o and its argument with argv_array_pushl() to make it obvious that they belong together. The resulting code is shorter and easier to extend. Helped-by: Eric Sunshine Signed-off-by: Rene Scharfe --- Change from v1: use argv_array_pushl for -o and tmp_idx. Thanks, Eric, good idea! http.c | 11 +++ 1 file changed, 3 insertions(+), 8 deletions(-) diff --git a/http.c b/http.c index 215bebef1b..9f2fcc9ec4 100644 --- a/http.c +++ b/http.c @@ -2025,7 +2025,6 @@ int finish_http_pack_request(struct http_pack_request *preq) char *tmp_idx; size_t len; struct child_process ip = CHILD_PROCESS_INIT; - const char *ip_argv[8]; close_pack_index(p); @@ -2041,13 +2040,9 @@ int finish_http_pack_request(struct http_pack_request *preq) die("BUG: pack tmpfile does not end in .pack.temp?"); tmp_idx = xstrfmt("%.*s.idx.temp", (int)len, preq->tmpfile); - ip_argv[0] = "index-pack"; - ip_argv[1] = "-o"; - ip_argv[2] = tmp_idx; - ip_argv[3] = preq->tmpfile; - ip_argv[4] = NULL; - - ip.argv = ip_argv; + argv_array_push(&ip.args, "index-pack"); + argv_array_pushl(&ip.args, "-o", tmp_idx, NULL); + argv_array_push(&ip.args, preq->tmpfile); ip.git_cmd = 1; ip.no_stdin = 1; ip.no_stdout = 1; -- 2.15.1
[PATCH] sequencer: assign only free()able strings to gpg_sign
The gpg_sign member of the replay_opts structure is of type `char *`, meaning that the sequencer deems the string to which gpg_sign points to be under its custody, i.e. it needs to be free()d by the sequencer. Therefore, let's only assign malloc()ed buffers to it. Reported-by: Kaartic Sivaraam Signed-off-by: Johannes Schindelin --- Phillip, if you want to squash these changes into your patches, I'd totally fine with that. Based-On: pw/sequencer-in-process-commit at https://github.com/dscho/git Fetch-Base-Via: git fetch https://github.com/dscho/git pw/sequencer-in-process-commit Published-As: https://github.com/dscho/git/releases/tag/sequencer-owns-gpg-sign-v1 Fetch-It-Via: git fetch https://github.com/dscho/git sequencer-owns-gpg-sign-v1 sequencer.c | 2 +- t/t3404-rebase-interactive.sh | 10 ++ 2 files changed, 11 insertions(+), 1 deletion(-) diff --git a/sequencer.c b/sequencer.c index 7051b20b762..1b2599668f5 100644 --- a/sequencer.c +++ b/sequencer.c @@ -160,7 +160,7 @@ static int git_sequencer_config(const char *k, const char *v, void *cb) } if (!strcmp(k, "commit.gpgsign")) { - opts->gpg_sign = git_config_bool(k, v) ? "" : NULL; + opts->gpg_sign = git_config_bool(k, v) ? xstrdup("") : NULL; return 0; } diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh index 9ed0a244e6c..040ef1a4dbc 100755 --- a/t/t3404-rebase-interactive.sh +++ b/t/t3404-rebase-interactive.sh @@ -1318,6 +1318,16 @@ test_expect_success 'editor saves as CR/LF' ' SQ="'" test_expect_success 'rebase -i --gpg-sign=' ' + test_when_finished "test_might_fail git rebase --abort" && + set_fake_editor && + FAKE_LINES="edit 1" git rebase -i --gpg-sign="\"S I Gner\"" HEAD^ \ + >out 2>err && + test_i18ngrep "$SQ-S\"S I Gner\"$SQ" err +' + +test_expect_success 'rebase -i --gpg-sign= overrides commit.gpgSign' ' + test_when_finished "test_might_fail git rebase --abort" && + test_config commit.gpgsign true && set_fake_editor && FAKE_LINES="edit 1" git rebase -i --gpg-sign="\"S I Gner\"" HEAD^ \ >out 2>err && base-commit: 28d6daed4f119940ace31e523b3b272d3d153d04 -- 2.15.1.windows.2
Re: Error in `git': free(): invalid pointer (was Re: [PATCH] sequencer: improve config handling)
Hi Kaartic, On Thu, 21 Dec 2017, Kaartic Sivaraam wrote: > Speaking of trace, (thanks to Dscho!) the one I got using 'valgrind' > (with `--leak-check=full` option) can be found below. I've kept only > relevant parts of it to avoid unwanted noise of `set -x`. Let me know > if that's needed too. > > ... > > + git diff-files --quiet --ignore-submodules > + git diff-index --cached --quiet --ignore-submodules HEAD -- > + test 0 = 1 > + test -z 1 > + valgrind --leak-check=full git rebase--helper --continue > ==10384== Memcheck, a memory error detector > ==10384== Copyright (C) 2002-2015, and GNU GPL'd, by Julian Seward et al. > ==10384== Using Valgrind-3.12.0.SVN and LibVEX; rerun with -h for copyright > info > ==10384== Command: git rebase--helper --continue > ==10384== > ==10384== Invalid free() / delete / delete[] / realloc() > ==10384==at 0x4C2CDDB: free (in > /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so) > ==10384==by 0x24E3F8: read_populate_opts (sequencer.c:1964) > ==10384==by 0x24E3F8: sequencer_continue (sequencer.c:2753) > ==10384==by 0x1735DC: cmd_rebase__helper (rebase--helper.c:52) > ==10384==by 0x11B847: run_builtin (git.c:346) > ==10384==by 0x11B847: handle_builtin (git.c:554) > ==10384==by 0x11BB05: run_argv (git.c:606) > ==10384==by 0x11BB05: cmd_main (git.c:683) > ==10384==by 0x11AC0B: main (common-main.c:43) > ==10384== Address 0x2a898a is in a r-x mapped file > /mnt/Source/Git/git-next/git segment > ==10384== > Successfully rebased and updated refs/heads/public. > ==10384== > ==10384== HEAP SUMMARY: > ==10384== in use at exit: 680,882 bytes in 332 blocks > ==10384== total heap usage: 717 allocs, 386 frees, 1,709,293 bytes allocated > ==10384== > ==10384== 2,053 (2,048 direct, 5 indirect) bytes in 1 blocks are definitely > lost in loss record 75 of 83 > ==10384==at 0x4C2BADF: malloc (in > /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so) > ==10384==by 0x4C2DE5F: realloc (in > /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so) > ==10384==by 0x27E0FE: xrealloc (wrapper.c:138) > ==10384==by 0x2072A3: add_object_array_with_path (object.c:319) > ==10384==by 0x23D745: add_pending_object_with_path (revision.c:163) > ==10384==by 0x24030E: add_pending_object_with_mode (revision.c:170) > ==10384==by 0x24030E: add_pending_object (revision.c:176) > ==10384==by 0x24030E: add_head_to_pending (revision.c:188) > ==10384==by 0x280EFA: has_uncommitted_changes.part.17 (wt-status.c:2288) > ==10384==by 0x24DF88: commit_staged_changes (sequencer.c:2705) > ==10384==by 0x24DF88: sequencer_continue (sequencer.c:2749) > ==10384==by 0x1735DC: cmd_rebase__helper (rebase--helper.c:52) > ==10384==by 0x11B847: run_builtin (git.c:346) > ==10384==by 0x11B847: handle_builtin (git.c:554) > ==10384==by 0x11BB05: run_argv (git.c:606) > ==10384==by 0x11BB05: cmd_main (git.c:683) > ==10384==by 0x11AC0B: main (common-main.c:43) > ==10384== > ==10384== LEAK SUMMARY: > ==10384==definitely lost: 2,048 bytes in 1 blocks > ==10384==indirectly lost: 5 bytes in 1 blocks > ==10384== possibly lost: 0 bytes in 0 blocks > ==10384==still reachable: 678,829 bytes in 330 blocks > ==10384== suppressed: 0 bytes in 0 blocks > ==10384== Reachable blocks (those to which a pointer was found) are not shown. > ==10384== To see them, rerun with: --leak-check=full --show-leak-kinds=all > ==10384== > ==10384== For counts of detected and suppressed errors, rerun with: -v > ==10384== ERROR SUMMARY: 2 errors from 2 contexts (suppressed: 0 from 0) > + exit > > > I think I didn't mention I've set `commit.gpgsign` to `true` for that > repo, did I? Hah! I had troubles to associate the correct line in my versions of Git's source code (the line numbers alone are only reliable if you also have a commit from which the Git binaries were built), but I did this free() at (https://github.com/git/git/blob/cd54ea2b18/sequencer.c#L1975: if (read_oneliner(&buf, rebase_path_gpg_sign_opt(), 1)) { if (!starts_with(buf.buf, "-S")) strbuf_reset(&buf); else { free(opts->gpg_sign); ^ opts->gpg_sign = xstrdup(buf.buf + 2); } strbuf_reset(&buf); } The culprit is that we have these really unclear ownership rules, where it is not at all clear who is responsible for releasing allocated memory: caller or callee? (Hannes would not rightfully point out that this would be a non-issue if we would switch to C++). In C, the common pattern is to use `const char *` for users that are *not* supposed to take ownership, and `char *` for users that are supposed to take custody. And in this instance, `gpg_sign` should be owned by `struct replay_opts` because of this (https://github.com/git/git/blob/cd54ea2b18/sequencer.h#L38): char *g
[PATCH] oidmap.h: strongly discourage using OIDMAP_INIT directly
In Git's source code, we have this convention that quite a few data structures can be initialized using a macro *_INIT while defining an instance (instead of having to call a function to initialize the data structure). You will see that idiom quite a bit, e.g. `struct strbuf buf = STRBUF_INIT;` This works for oidsets, too: `struct oidset oids = OIDSET_INIT;` is perfectly legal and you can use that data structure right away, without having to call `oidset_init()` first. That pattern is violated by OIDMAP_INIT, though. The first call to oidmap_put() or oidmap_get() will succeed, but by mistake rather than by design: The underlying hashmap is not initialized correctly, as the cmpfn function pointer still points to NULL, but since there are no entries to be compared, cmpfn will not be called. Things break down, though, as soon as there is even one entry. Rather than causing confusion, frustration and needless loss of time due to pointless debugging, let's *rename* OIDMAP_INIT so that developers who have gotten used to the pattern `struct xyz a = XYZ_INIT;` won't do that with oidmaps. An alternative would be to introduce a HASHMAP_INIT(cmp_fn) and use that in oidmap.h. However, there are *no* call sites in Git's source code that would benefit from that macro, i.e. this would be premature optimization (and cost a lot more time than this here trivial patch). Signed-off-by: Johannes Schindelin --- Published-As: https://github.com/dscho/git/releases/tag/discourage-OIDMAP_INIT-v1 Fetch-It-Via: git fetch https://github.com/dscho/git discourage-OIDMAP_INIT-v1 oidmap.h | 6 +- oidset.h | 7 ++- 2 files changed, 11 insertions(+), 2 deletions(-) diff --git a/oidmap.h b/oidmap.h index 18f54cde143..1a73c392b79 100644 --- a/oidmap.h +++ b/oidmap.h @@ -21,7 +21,11 @@ struct oidmap { struct hashmap map; }; -#define OIDMAP_INIT { { NULL } } +/* + * This macro initializes the data structure only incompletely, just enough + * to call oidmap_get() on an empty map. Use oidmap_init() instead. + */ +#define OIDMAP_INIT_INCOMPLETELY { { NULL } } /* * Initializes an oidmap structure. diff --git a/oidset.h b/oidset.h index f4c9e0f9c04..a11d88edc1d 100644 --- a/oidset.h +++ b/oidset.h @@ -22,7 +22,12 @@ struct oidset { struct oidmap map; }; -#define OIDSET_INIT { OIDMAP_INIT } +/* + * It is okay to initialize the map incompletely here because oidset_insert() + * will call oidset_init() (which will call oidmap_init()), and + * oidset_contains() works as intended even before oidset_init() was called. + */ +#define OIDSET_INIT { OIDMAP_INIT_INCOMPLETELY } /** * Returns true iff `set` contains `oid`. base-commit: 936d1b989416a95f593bf81ccae8ac62cd83f279 -- 2.15.1.windows.2
Re: Error in `git': free(): invalid pointer (was Re: [PATCH] sequencer: improve config handling)
>Original Message >From: kaartic.sivar...@gmail.com >Date: 21/12/2017 17:14 >To: "phillip.w...@talktalk.net", "Phillip Wood", "Git Mailing List" >Cc: "Johannes Schindelin" >Subj: Re: Error in `git': free(): invalid pointer (was Re: [PATCH] sequencer: improve config handling) > >On Thu, 2017-12-21 at 16:53 +, phillip.w...@talktalk.net wrote: >> Hm, There is a problem with sequencer_remove_state() which does >> >> free(opts->gpg_sign) >> >> however unless a gpg key was given on the commandline, opts->gpg is >> initialized to "" which is statically allocated. >> >> This untested diff should fix that, > >It did seem to. I don't get that error any more. That's good, thanks for testing it >> but I'm not sure if you're problem >> is related to it > >As the fix you suggested did work, I think the problem is related. Did >you have anything else in mind when you said you're not sure whether or >not it's related? I didn't have anything else in mind, but at that point I hadn't noticed that one of your previous messages said you had gpg signing turned on - as you do it makes sense that the patch fixed it. Thanks again for reporting this and testing the fix - it seems the test suite has a bit of a blind spot when it comes to gpg signing, I guess it's difficult to set up a key for tests I'll send a proper patch when I have time in a few days Best Wishes Phillip
Re: [PATCH] http: use internal argv_array of struct child_process
On Fri, Dec 22, 2017 at 3:14 AM, René Scharfe wrote: > Avoid a strangely magic array size (it's slightly too big) and explicit > index numbers by building the command line for index-pack using the > embedded argv_array of the child_process. The resulting code is shorter > and easier to extend. > > Signed-off-by: Rene Scharfe > --- > diff --git a/http.c b/http.c > @@ -2041,13 +2040,10 @@ int finish_http_pack_request(struct http_pack_request > *preq) > - ip_argv[0] = "index-pack"; > - ip_argv[1] = "-o"; > - ip_argv[2] = tmp_idx; > - ip_argv[3] = preq->tmpfile; > - ip_argv[4] = NULL; > - > - ip.argv = ip_argv; > + argv_array_push(&ip.args, "index-pack"); > + argv_array_push(&ip.args, "-o"); > + argv_array_push(&ip.args, tmp_idx); > + argv_array_push(&ip.args, preq->tmpfile); Not necessarily worth a re-roll, but using the "pushl" variant would make it clear that "-o" and tmp_idx are related and would ensure that they don't accidentally get split up if someone inserts a new "push" in the sequence in the future. argv_array_push(&ip.args, "index-pack"); argv_array_pushl(&ip.args, "-o", tmp_idx, NULL); argv_array_push(&ip.args, preq->tmpfile);
Assalamu`Alaikum.
Greetings from Dr. mohammad ouattara. Assalamu`Alaikum. My Name is Dr. mohammad ouattara, I am a banker by profession. I'm from Ouagadougou, Burkina Faso, West Africa. My reason for contacting you is to transfer an abandoned $14.6M to your account. The owner of this fund died since 2004 with his Next Of Kin. I want to present you to the bank as the Next of Kin/beneficiary of this fund. Further details of the transaction shall be forwarded to you as soon as I receive your return mail indicating your interest. 1) YOUR FULL NAME... (2) YOUR AGE AND SEX (3) YOUR CONTACT ADDRESS.. (4) YOUR PRIVATE PHONE N0.. (5) FAX NUMBER.. (6) YOUR COUNTRY OF ORIGIN.. (7) YOUR OCCUPATION. Have a great day, Dr. mohammad ouattara.
[PATCH] send-pack: use internal argv_array of struct child_process
Avoid a magic number of NULL placeholder values and a magic index by constructing the command line for pack-objects using the embedded argv_array of the child_process. The resulting code is shorter and easier to extend. Signed-off-by: Rene Scharfe --- send-pack.c | 28 +--- 1 file changed, 9 insertions(+), 19 deletions(-) diff --git a/send-pack.c b/send-pack.c index a8cc6b266e..2112d3b27a 100644 --- a/send-pack.c +++ b/send-pack.c @@ -58,35 +58,25 @@ static int pack_objects(int fd, struct ref *refs, struct oid_array *extra, struc * the revision parameters to it via its stdin and * let its stdout go back to the other end. */ - const char *argv[] = { - "pack-objects", - "--all-progress-implied", - "--revs", - "--stdout", - NULL, - NULL, - NULL, - NULL, - NULL, - NULL, - }; struct child_process po = CHILD_PROCESS_INIT; FILE *po_in; int i; int rc; - i = 4; + argv_array_push(&po.args, "pack-objects"); + argv_array_push(&po.args, "--all-progress-implied"); + argv_array_push(&po.args, "--revs"); + argv_array_push(&po.args, "--stdout"); if (args->use_thin_pack) - argv[i++] = "--thin"; + argv_array_push(&po.args, "--thin"); if (args->use_ofs_delta) - argv[i++] = "--delta-base-offset"; + argv_array_push(&po.args, "--delta-base-offset"); if (args->quiet || !args->progress) - argv[i++] = "-q"; + argv_array_push(&po.args, "-q"); if (args->progress) - argv[i++] = "--progress"; + argv_array_push(&po.args, "--progress"); if (is_repository_shallow()) - argv[i++] = "--shallow"; - po.argv = argv; + argv_array_push(&po.args, "--shallow"); po.in = -1; po.out = args->stateless_rpc ? -1 : fd; po.git_cmd = 1; -- 2.15.1
[PATCH] http: use internal argv_array of struct child_process
Avoid a strangely magic array size (it's slightly too big) and explicit index numbers by building the command line for index-pack using the embedded argv_array of the child_process. The resulting code is shorter and easier to extend. Signed-off-by: Rene Scharfe --- http.c | 12 1 file changed, 4 insertions(+), 8 deletions(-) diff --git a/http.c b/http.c index 215bebef1b..b22b4ada58 100644 --- a/http.c +++ b/http.c @@ -2025,7 +2025,6 @@ int finish_http_pack_request(struct http_pack_request *preq) char *tmp_idx; size_t len; struct child_process ip = CHILD_PROCESS_INIT; - const char *ip_argv[8]; close_pack_index(p); @@ -2041,13 +2040,10 @@ int finish_http_pack_request(struct http_pack_request *preq) die("BUG: pack tmpfile does not end in .pack.temp?"); tmp_idx = xstrfmt("%.*s.idx.temp", (int)len, preq->tmpfile); - ip_argv[0] = "index-pack"; - ip_argv[1] = "-o"; - ip_argv[2] = tmp_idx; - ip_argv[3] = preq->tmpfile; - ip_argv[4] = NULL; - - ip.argv = ip_argv; + argv_array_push(&ip.args, "index-pack"); + argv_array_push(&ip.args, "-o"); + argv_array_push(&ip.args, tmp_idx); + argv_array_push(&ip.args, preq->tmpfile); ip.git_cmd = 1; ip.no_stdin = 1; ip.no_stdout = 1; -- 2.15.1