Speed of git branch --contains
Hi everybody, I'm just looking at some scripts that do a 'git branch --contains $id --remote' for each new commit in a repo, and unfortunately each invokation already takes four minutes. It feels like git branch does the reachability detection separately for each branch potentially listed. The alternative would be to - invert the parent map to a child map, - use that to compute the set of commits that contain $id, - then use that as predicate whether to show a given branch (show iff its head is in the set) That would speed things up considerably, but what are the chances to see that change in git? I can do that as well within the script, with the additional benefit that I only need to do the inversion once, but I might instead take a stab at git branch. - Andreas -- "Totally trivial. Famous last words." From: Linus TorvaldsDate: Fri, 22 Jan 2010 07:29:21 -0800
Problem with git and proxy server with authentication
Hi. (sory for my english - it's not native for me) I use proxy-server with authentication. I have two instances of PortableGit: version 2.10.0.windows.1 and 2.16.1.windows.1 I added proxy settings in ~/.gitconfig. If I try to pull my repository with version 2.16.1.windows.1 , I get error message: "fatal: unable to access 'https://aleksey_yaroslav...@bitbucket.org/aleksey_yaroslavcev/testgitflow.git/': Proxy CONNECT aborted" But pulling with version 2.10.0.windows.1 working fine with the same settings in ~/.gitconfig. I decided to figure out what the problem is and found out the following. There is a difference in the way how git making connection through proxy. See the attached screenshots from WireShark. In old version: 1. creating connection to proxy 2. trying to connect without authentication 3. proxy saying "407 Authentication Required" and requesting termination of connection(FIN flag in TCP packet) 4. reseting conecction(RST flag in TCP packet) 5. creating another connection 6. trying to connect with authentication 7. Bingo! In new version: 1. creating connection to proxy 2. trying to connect without authentication 3. proxy saying "407 Authentication Required" and requesting termination of connection (FIN flag in TCP packet) 4. trying to connect with authentication in the same connection but proxy is resetting connection (RST flag in TCP packet)
Re: [PATCH v6] mru: Replace mru.[ch] with list.h implementation
On Tue, Jan 23, 2018 at 06:46:51PM -0500, Gargi Sharma wrote: > Replace the custom calls to mru.[ch] with calls to list.h. This patch is > the final step in removing the mru API completely and inlining the logic. > This patch leads to significant code reduction and the mru API hence, is > not a useful abstraction anymore. > > Signed-off-by: Gargi SharmaThanks, this version looks good to me. -Peff
Re: [PATCH] mailinfo: avoid segfault when can't open files
On Tue, Jan 23, 2018 at 11:54:17PM -0300, Juan F. Codagnone wrote: > If or files can't be opened, clear_mailinfo crash as > it follows NULL pointers. > > Can be reproduced using `git mailinfo . .` Thanks for finding this. Looking at the offending code and your solution, it looks like: 1. mailinfo() sometimes allocates mi->p_hdr_data and mi->s_hdr_data and sometimes not, depending on how far we get before seeing an error. The caller cannot know whether we did so or not based on seeing an error return, but most call clear_mailinfo() if it wants to avoid a leak. 2. There are two callers of mailinfo(). git-am simply dies on an error, and so is unaffected. But git-mailinfo unconditionally calls clear_mailinfo() before returning, regardless of the return code. 3. When we get to clear_mailinfo(), the arrays are either populated or are NULL. We know they're initialized to NULL because of setup_mailinfo(), which zeroes the whole struct. So I think your fix does the right thing. I do think this is a pretty awkward interface, and it would be less error-prone if either[1]: a. we bumped the allocation of these arrays up in mailinfo() so that they were simply always initialized. This fixes the bug in clear_mailinfo(), but also any other function which looks at the mailinfo struct (though I don't think there are any such cases). b. we had mailinfo() clean up after itself on error, so that it was always in a de-initialized state. But given the lack of callers, it may not be worth the effort. So I'm OK with this solution. It may be worth giving an abbreviated version of the above explanation in the commit message. Perhaps: If or files can't be opened, then mailinfo() returns an error before it even initializes mi->p_hdr_data or mi->s_hdr_data. When cmd_mailinfo() then calls clear_mailinfo(), we dereference the NULL pointers trying to free their contents. As for the patch itself, it looks correct but I saw two style nits: > diff --git a/mailinfo.c b/mailinfo.c > index a89db22ab..035abbbf5 100644 > --- a/mailinfo.c > +++ b/mailinfo.c > @@ -1167,11 +1167,13 @@ void clear_mailinfo(struct mailinfo *mi) > strbuf_release(>inbody_header_accum); > free(mi->message_id); > > - for (i = 0; mi->p_hdr_data[i]; i++) > - strbuf_release(mi->p_hdr_data[i]); > + if(mi->p_hdr_data != NULL) > + for (i = 0; mi->p_hdr_data[i]; i++) > + strbuf_release(mi->p_hdr_data[i]); We usually say "if (" with an extra space. And we generally just check pointers for their truth value. So: if (mi->p_hdr_data) { for (i = 0; ...) -Peff [1] Actually, it seems a little funny that we use xcalloc() here at all, since the size is determined by a compile-time constant. Why not just put an array directly into the struct and let it get zeroed with the rest of the struct? That sidesteps the question of whether we need to clear() after an error return, but it would fix this bug. :)
[PATCH] mailinfo: avoid segfault when can't open files
If or files can't be opened, clear_mailinfo crash as it follows NULL pointers. Can be reproduced using `git mailinfo . .` Signed-off-by: Juan F. Codagnone--- mailinfo.c | 10 ++ 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/mailinfo.c b/mailinfo.c index a89db22ab..035abbbf5 100644 --- a/mailinfo.c +++ b/mailinfo.c @@ -1167,11 +1167,13 @@ void clear_mailinfo(struct mailinfo *mi) strbuf_release(>inbody_header_accum); free(mi->message_id); - for (i = 0; mi->p_hdr_data[i]; i++) - strbuf_release(mi->p_hdr_data[i]); + if(mi->p_hdr_data != NULL) + for (i = 0; mi->p_hdr_data[i]; i++) + strbuf_release(mi->p_hdr_data[i]); free(mi->p_hdr_data); - for (i = 0; mi->s_hdr_data[i]; i++) - strbuf_release(mi->s_hdr_data[i]); + if(mi->s_hdr_data != NULL) + for (i = 0; mi->s_hdr_data[i]; i++) + strbuf_release(mi->s_hdr_data[i]); free(mi->s_hdr_data); while (mi->content < mi->content_top) { -- 2.14.3
Your long awaited part payment of $2.5.000.00Usd
Attention: Beneficiary, Your long awaited part payment of $2.5.000.00Usd (TWO MILLION FIVE Hundred Thousand United State Dollars) is ready for immediate release to you, and it was electronically credited into an ATM Visa Card for easy delivery. Your new Payment Reference No.- 6363836, Password No: 006786, Pin Code No: 1787 Your Certificate of Merit Payment No: 05872, Released Code No: 1134; Immediate Telex confirmation No:- 043612; Secret Code No: TBKTA28 Re-Confirm; Your Names: |Address: |Cell Phone: |Fax: | Amount: | Your immediate response is needed UBA Bank Person to Contact:MR KELLY HALL the Director of the International Audit unit ATM Payment Center, Email: ubabf_uba...@financier.com TELEPHONE: +226 68251393 Note: Your file will expire after 14 days if there is no response, and then we will not have any option than to return your fund to IMF as unclaimed. Your swift response will enhance our service, thanks for your co-operation; Regards. Mr JOHN MARK.
Re: [PATCH 5/5] travis-ci: don't fail if user already exists on 32 bit Linux build job
On Tue, Jan 23, 2018 at 11:46 PM, Jeff Kingwrote: >> The build job on Travis CI always starts with a fresh Docker >> container, so this change doesn't make a difference there. > > I wonder if that is fixable. Installing dependencies into the container > takes quite a lot of time, and is just wasted effort. I agree (because apt-get install phase took forever on my laptop). I've seen people just include a Dockerfile that builds everything in (except things like creating user matching host). First try to fetch it from docker hub (or a local registry but I don't think it applies to us). If not found, rebuild the image with Dockerfile and run a new container with it. Every time somebody changes dockerfile, they are supposed to step the image version up. Of course this is still wasted effort unless somebody pushes images to docker registry and the script has to rebuild the image every single time [1]. Somebody could do that manually. I see docker hub has some automated rebuild feature, perhaps that'll work for us. [1] It still benefits users who run ci scripts manually because the created image will stay in their local machine. -- Duy
[PATCH v6] mru: Replace mru.[ch] with list.h implementation
Replace the custom calls to mru.[ch] with calls to list.h. This patch is the final step in removing the mru API completely and inlining the logic. This patch leads to significant code reduction and the mru API hence, is not a useful abstraction anymore. Signed-off-by: Gargi Sharma--- The commit has been built on top of ot/mru-on-list branch. Changes in v6: - Replace mru_clear with INIT_LIST_HEAD() to reset the mru list. Changes in v5: - Remove list_move_to_front() as it is redundant. Changes in v4: - Fixing minor style issues. Changes in v3: - Make the commit message more descriptive. - Remove braces after if statement. Changes in v2: - Add a move to front function to the list API. - Remove memory leak. - Remove redundant remove operations on the list. A future improvement could be removing/changing the type of nect pointer or dropping it entirely. See discussion here: https://public-inbox.org/git/caoci2dgyqr4jff5oby2buyhnjeaapqkf8tbojn2w0b18eo+...@mail.gmail.com/ --- --- Makefile | 1 - builtin/pack-objects.c | 9 - cache.h| 8 mru.c | 27 --- mru.h | 40 packfile.c | 17 + sha1_file.c| 1 - 7 files changed, 17 insertions(+), 86 deletions(-) delete mode 100644 mru.c delete mode 100644 mru.h diff --git a/Makefile b/Makefile index ed4ca43..4a79ec5 100644 --- a/Makefile +++ b/Makefile @@ -814,7 +814,6 @@ LIB_OBJS += merge.o LIB_OBJS += merge-blobs.o LIB_OBJS += merge-recursive.o LIB_OBJS += mergesort.o -LIB_OBJS += mru.o LIB_OBJS += name-hash.o LIB_OBJS += notes.o LIB_OBJS += notes-cache.o diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c index ba81234..6a0b8e8 100644 --- a/builtin/pack-objects.c +++ b/builtin/pack-objects.c @@ -24,7 +24,7 @@ #include "reachable.h" #include "sha1-array.h" #include "argv-array.h" -#include "mru.h" +#include "list.h" #include "packfile.h" static const char *pack_usage[] = { @@ -1012,9 +1012,8 @@ static int want_object_in_pack(const unsigned char *sha1, return want; } - list_for_each(pos, _git_mru.list) { - struct mru *entry = list_entry(pos, struct mru, list); - struct packed_git *p = entry->item; + list_for_each(pos, _git_mru) { + struct packed_git *p = list_entry(pos, struct packed_git, mru); off_t offset; if (p == *found_pack) @@ -1031,7 +1030,7 @@ static int want_object_in_pack(const unsigned char *sha1, } want = want_found_object(exclude, p); if (!exclude && want > 0) - mru_mark(_git_mru, entry); + list_move(>mru, _git_mru); if (want != -1) return want; } diff --git a/cache.h b/cache.h index 49b083e..cc09e3b 100644 --- a/cache.h +++ b/cache.h @@ -4,7 +4,7 @@ #include "git-compat-util.h" #include "strbuf.h" #include "hashmap.h" -#include "mru.h" +#include "list.h" #include "advice.h" #include "gettext.h" #include "convert.h" @@ -1566,6 +1566,7 @@ struct pack_window { extern struct packed_git { struct packed_git *next; + struct list_head mru; struct pack_window *windows; off_t pack_size; const void *index_data; @@ -1587,10 +1588,9 @@ extern struct packed_git { } *packed_git; /* - * A most-recently-used ordered version of the packed_git list, which can - * be iterated instead of packed_git (and marked via mru_mark). + * A most-recently-used ordered version of the packed_git list. */ -extern struct mru packed_git_mru; +extern struct list_head packed_git_mru; struct pack_entry { off_t offset; diff --git a/mru.c b/mru.c deleted file mode 100644 index 8f3f34c..000 --- a/mru.c +++ /dev/null @@ -1,27 +0,0 @@ -#include "cache.h" -#include "mru.h" - -void mru_append(struct mru *head, void *item) -{ - struct mru *cur = xmalloc(sizeof(*cur)); - cur->item = item; - list_add_tail(>list, >list); -} - -void mru_mark(struct mru *head, struct mru *entry) -{ - /* To mark means to put at the front of the list. */ - list_del(>list); - list_add(>list, >list); -} - -void mru_clear(struct mru *head) -{ - struct list_head *pos; - struct list_head *tmp; - - list_for_each_safe(pos, tmp, >list) { - free(list_entry(pos, struct mru, list)); - } - INIT_LIST_HEAD(>list); -} diff --git a/mru.h b/mru.h deleted file mode 100644 index 80a589e..000 --- a/mru.h +++ /dev/null @@ -1,40 +0,0 @@ -#ifndef MRU_H -#define MRU_H - -#include "list.h" - -/** - * A simple most-recently-used cache, backed by a doubly-linked list. - * - *
What's cooking in git.git (Jan 2018, #03; Tue, 23)
Here are the topics that have been cooking. Commits prefixed with '-' are only in 'pu' (proposed updates) while commits prefixed with '+' are in 'next'. The ones marked with '.' do not appear in any of the integration branches, but I am still holding onto them. Many topics in 'next' have been merged to 'master' while others are tentatively kicked back to 'pu', to give them a chance to be rerolled if the authors of them wish to do so. You can find the changes described here in the integration branches of the repositories listed at http://git-blame.blogspot.com/p/git-public-repositories.html -- [Graduated to "master"] * ab/commit-m-with-fixup (2017-12-22) 2 commits (merged to 'next' on 2018-01-11 at 41255c464f) + commit: add support for --fixup -m"" + commit doc: document that -c, -C, -F and --fixup with -m error "git commit --fixup" did not allow "-m" option to be used at the same time; allow it to annotate resulting commit with more text. * ab/doc-cat-file-e-still-shows-errors (2018-01-10) 1 commit (merged to 'next' on 2018-01-12 at 080bb1d397) + cat-file doc: document that -e will return some output Doc update. * ab/perf-grep-threads (2018-01-04) 1 commit (merged to 'next' on 2018-01-09 at 91889574fb) + perf: amend the grep tests to test grep.threads More perf tests for threaded grep * as/read-tree-prefix-doc-fix (2018-01-09) 1 commit (merged to 'next' on 2018-01-12 at 895c72e5c3) + doc/read-tree: remove obsolete remark Doc update. * bw/oidmap-autoinit (2017-12-27) 1 commit (merged to 'next' on 2018-01-11 at f941e013b4) + oidmap: ensure map is initialized Code clean-up. * cc/codespeed (2018-01-05) 7 commits (merged to 'next' on 2018-01-09 at 8578089a2b) + perf/run: read GIT_PERF_REPO_NAME from perf.repoName + perf/run: learn to send output to codespeed server + perf/run: learn about perf.codespeedOutput + perf/run: add conf_opts argument to get_var_from_env_or_config() + perf/aggregate: implement codespeed JSON output + perf/aggregate: refactor printing results + perf/aggregate: fix checking ENV{GIT_PERF_SUBSECTION} "perf" test output can be sent to codespeed server. * dk/describe-all-output-fix (2017-12-27) 1 commit (merged to 'next' on 2017-12-28 at c6254494e3) + describe: prepend "tags/" when describing tags with embedded name An old regression in "git describe --all $annotated_tag^0" has been fixed. * ew/empty-merge-with-dirty-index (2018-01-09) 1 commit (merged to 'next' on 2018-01-09 at 6bcda11248) + Merge branch 'ew/empty-merge-with-dirty-index-maint' into ew/empty-merge-with-dirty-index (this branch uses ew/empty-merge-with-dirty-index-maint.) "git merge -s recursive" did not correctly abort when the index is dirty, if the merged tree happened to be the same as the current HEAD, which has been fixed. * jc/merge-symlink-ours-theirs (2018-01-03) 1 commit (merged to 'next' on 2018-01-05 at 63ebfc45eb) + merge: teach -Xours/-Xtheirs to symbolic link merge "git merge -Xours/-Xtheirs" learned to use our/their version when resolving a conflicting updates to a symbolic link. * jh/object-filtering (2018-01-08) 1 commit (merged to 'next' on 2018-01-11 at 56808f6969) + oidset: don't return value from oidset_init Hotfix for a topic already in 'master'. * jk/abort-clone-with-existing-dest (2018-01-03) 4 commits (merged to 'next' on 2018-01-09 at 3c8e83c3a7) + clone: do not clean up directories we didn't create + clone: factor out dir_exists() helper + t5600: modernize style + t5600: fix outdated comment about unborn HEAD "git clone $there $here" is allowed even when here directory exists as long as it is an empty directory, but the command incorrectly removed it upon a failure of the operation. * jm/svn-pushmergeinfo-fix (2017-09-17) 1 commit (merged to 'next' on 2018-01-05 at 6cb237ea44) + git-svn: fix svn.pushmergeinfo handling of svn+ssh usernames. "git svn dcommit" did not take into account the fact that a svn+ssh:// URL with a username@ (typically used for pushing) refers to the same SVN repository without the username@ and failed when svn.pushmergeinfo option is set. * js/fix-merge-arg-quoting-in-rebase-p (2018-01-05) 1 commit (merged to 'next' on 2018-01-09 at 91f5601e9c) + rebase -p: fix quoting when calling `git merge` "git rebase -p -X" did not propagate the option properly down to underlying merge strategy backend. * js/test-with-ws-in-path (2018-01-10) 1 commit (merged to 'next' on 2018-01-10 at c44db26fe4) + t3900: add some more quotes Hot fix to a test. * ma/bisect-leakfix (2018-01-03) 1 commit (merged to 'next' on 2018-01-09 at 2ef8b59d1b) + bisect: fix a regression causing a segfault A hotfix for a recent update that broke 'git bisect'. * mm/send-email-fallback-to-local-mail-address (2018-01-08) 3 commits (merged to 'next' on 2018-01-17 at dd4867706b) + send-email: add test for Linux's get_maintainer.pl
[PATCH v3 04/11] fetch tests: re-arrange arguments for future readability
Re-arrange the arguments to the test_configured_prune() function used in this test to pass the arguments to --fetch last. A subsequent change will test for more elaborate fetch arguments, including long refspecs. It'll be more readable to be able to wrap those on a new line of their own. Signed-off-by: Ævar Arnfjörð Bjarmason--- t/t5510-fetch.sh | 82 ++-- 1 file changed, 44 insertions(+), 38 deletions(-) diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh index 11da97f9b7..ab8b25344d 100755 --- a/t/t5510-fetch.sh +++ b/t/t5510-fetch.sh @@ -551,10 +551,10 @@ set_config_tristate () { test_configured_prune () { fetch_prune=$1 remote_origin_prune=$2 - cmdline=$3 - expected_branch=$4 + expected_branch=$3 + cmdline=$4 - test_expect_success "prune fetch.prune=$1 remote.origin.prune=$2${3:+ $3}; branch:$4" ' + test_expect_success "prune fetch.prune=$1 remote.origin.prune=$2${4:+ $4}; branch:$3" ' # make sure a newbranch is there in . and also in one git branch -f newbranch && ( @@ -587,41 +587,47 @@ test_configured_prune () { ' } -test_configured_prune unset unset "" kept -test_configured_prune unset unset "--no-prune" kept -test_configured_prune unset unset "--prune"pruned - -test_configured_prune false unset "" kept -test_configured_prune false unset "--no-prune" kept -test_configured_prune false unset "--prune"pruned - -test_configured_prune true unset "" pruned -test_configured_prune true unset "--prune"pruned -test_configured_prune true unset "--no-prune" kept - -test_configured_prune unset false "" kept -test_configured_prune unset false "--no-prune" kept -test_configured_prune unset false "--prune"pruned - -test_configured_prune false false "" kept -test_configured_prune false false "--no-prune" kept -test_configured_prune false false "--prune"pruned - -test_configured_prune true false "" kept -test_configured_prune true false "--prune"pruned -test_configured_prune true false "--no-prune" kept - -test_configured_prune unset true "" pruned -test_configured_prune unset true "--no-prune" kept -test_configured_prune unset true "--prune"pruned - -test_configured_prune false true "" pruned -test_configured_prune false true "--no-prune" kept -test_configured_prune false true "--prune"pruned - -test_configured_prune true true "" pruned -test_configured_prune true true "--prune"pruned -test_configured_prune true true "--no-prune" kept +# $1 config: fetch.prune +# $2 config: remote..prune +# $3 expect: branch to be pruned? +# $4 git-fetch $cmdline: +# +# $1$2$3 $4 +test_configured_prune unset unset kept "" +test_configured_prune unset unset kept "--no-prune" +test_configured_prune unset unset pruned "--prune" + +test_configured_prune false unset kept "" +test_configured_prune false unset kept "--no-prune" +test_configured_prune false unset pruned "--prune" + +test_configured_prune true unset pruned "" +test_configured_prune true unset pruned "--prune" +test_configured_prune true unset kept "--no-prune" + +test_configured_prune unset false kept "" +test_configured_prune unset false kept "--no-prune" +test_configured_prune unset false pruned "--prune" + +test_configured_prune false false kept "" +test_configured_prune false false kept "--no-prune" +test_configured_prune false false pruned "--prune" + +test_configured_prune true false kept "" +test_configured_prune true false pruned "--prune" +test_configured_prune true false kept "--no-prune" + +test_configured_prune unset true pruned "" +test_configured_prune unset true kept "--no-prune" +test_configured_prune unset true pruned "--prune" + +test_configured_prune false true pruned "" +test_configured_prune false true kept "--no-prune" +test_configured_prune false true pruned "--prune" + +test_configured_prune true true pruned "" +test_configured_prune true true pruned "--prune" +test_configured_prune true true kept "--no-prune" test_expect_success 'all boundary commits are excluded' ' test_commit base && -- 2.15.1.424.g9478a66081
[PATCH v3 08/11] git remote doc: correct dangerous lies about what prune does
The "git remote prune " command uses the same machinery as "git fetch --prune", and shares all the same caveats, but its documentation has suggested that it'll just "delete stale remote-tracking branches under ". This isn't true, and hasn't been true since at least v1.8.5.6 (the oldest version I could be bothered to test). E.g. if "+refs/tags/*:refs/tags/*" is explicitly set in the refspec of the remote it'll delete all local tags doesn't know about. Instead, briefly give the reader just enough of a hint that this option might constitute a shotgun aimed at their foot, and point them to the new PRUNING section in the git-fetch documentation which explains all the nuances of what this facility does. See "[BUG] git remote prune removes local tags, depending on fetch config" (caci5s_39wnrbfjlfn0xhcy+uewtfn2ymnacrc86z6pjutjw...@mail.gmail.com) by Michael Giuffrida for the initial report. Reported-by: Michael GiuffridaSigned-off-by: Ævar Arnfjörð Bjarmason --- Documentation/git-remote.txt | 14 +- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/Documentation/git-remote.txt b/Documentation/git-remote.txt index 577b969c1b..04e2410aec 100644 --- a/Documentation/git-remote.txt +++ b/Documentation/git-remote.txt @@ -172,10 +172,14 @@ With `-n` option, the remote heads are not queried first with 'prune':: -Deletes all stale remote-tracking branches under . -These stale branches have already been removed from the remote repository -referenced by , but are still locally available in -"remotes/". +Deletes stale references associated with . By default stale +remote-tracking branches under , but depending on global +configuration and the configuration of the remote we might even prune +local tags that haven't been pushed there. Equivalent to `git fetch +--prune `, except that no new references will be fetched. ++ +See the PRUNING section of linkgit:git-fetch[1] for what it'll prune +depending on various configuration. + With `--dry-run` option, report what branches will be pruned, but do not actually prune them. @@ -189,7 +193,7 @@ remotes.default is not defined, all remotes which do not have the configuration parameter remote..skipDefaultUpdate set to true will be updated. (See linkgit:git-config[1]). + -With `--prune` option, prune all the remotes that are updated. +With `--prune` option, run pruning against all the remotes that are updated. DISCUSSION -- 2.15.1.424.g9478a66081
[PATCH v3 00/11] document & test fetch pruning & add fetch.pruneTags
Addresses Junio's comments on v2 + more fixes. The Ævar Arnfjörð Bjarmason (11): fetch: don't redundantly NULL something calloc() gave us fetch: stop accessing "remote" variable indirectly Moved these patches to the beginning. No changes except a note to the commit message saying how these trivial changes relate (or not) to subsequent changes. fetch tests: refactor in preparation for testing tag pruning No changes. fetch tests: re-arrange arguments for future readability s/arrange/re-arrange/ in commit message subject. fetch tests: add a tag to be deleted to the pruning tests No changes. fetch tests: test --prune and refspec interaction I'm now just skipping quoting things like +refs/... on the command-line, which as grepping the rest of the test suite shows is fine, this eliminated the need for "fetch tests: double quote a variable for interpolation" so I've ejected it. git fetch doc: add a new section to explain the ins & outs of pruning No changes. git remote doc: correct dangerous lies about what prune does Rewording of doc per Junio's suggestion. git-fetch & config doc: link to the new PRUNING section No changes. fetch tests: add scaffolding for the new fetch.pruneTags No changes except continuing remove refspec quoting changes noted above & using +refs/tags/... instead of refs/tags/... for consistency with the last patch... fetch: add a --fetch-prune option and fetch.pruneTags config We now consistently use a + prefixed refspec for tag pruning, even though it yields the same result. See the discussion in 87tvvdh2vh@evledraar.gmail.com, I think it's less confusing. Fix regex in --replace-all command in the commit message. Rewording to address Junio's comments. The short help for --prune-tags is now: - N_("prune local tags not found on remote")), + N_("prune local tags no longer on remote and clobber changed tags")), Add --prune-tags to the bash completion. Documentation/config.txt | 20 - Documentation/fetch-options.txt| 18 +++- Documentation/git-fetch.txt| 76 + Documentation/git-remote.txt | 14 +-- builtin/fetch.c| 37 +++- contrib/completion/git-completion.bash | 2 +- remote.c | 5 +- remote.h | 3 + t/t5510-fetch.sh | 152 - 9 files changed, 275 insertions(+), 52 deletions(-) -- 2.15.1.424.g9478a66081
[PATCH v3 01/11] fetch: don't redundantly NULL something calloc() gave us
Stop redundantly NULL-ing the last element of the refs structure, which was retrieved via calloc(), and is thus guaranteed to be pre-NULL'd. This code dates back to b888d61c83 ("Make fetch a builtin", 2007-09-10), where wasn't any reason to do this back then either, it's just something left over from when git-fetch was initially introduced. The initial motivation for this change was to make a subsequent change which would also modify the refs variable smaller, since it won't have to copy this redundant "NULL the last + 1 item" pattern. As it turns out that change was bad, and better done differently, but as this pattern is still pointless let's fix it. Signed-off-by: Ævar Arnfjörð Bjarmason--- builtin/fetch.c | 1 - 1 file changed, 1 deletion(-) diff --git a/builtin/fetch.c b/builtin/fetch.c index 7bbcd26faf..b34665db9e 100644 --- a/builtin/fetch.c +++ b/builtin/fetch.c @@ -1302,7 +1302,6 @@ static int fetch_one(struct remote *remote, int argc, const char **argv) } else refs[j++] = argv[i]; } - refs[j] = NULL; ref_nr = j; } -- 2.15.1.424.g9478a66081
[PATCH v3 06/11] fetch tests: test --prune and refspec interaction
Add a test for the interaction between explicitly provided refspecs and fetch.prune. There's no point in adding this boilerplate to every combination of unset/false/true, it's instructive and sufficient to show that no matter if the variable is unset, false or true the refspec on the command-line overrides any configuration variable. Signed-off-by: Ævar Arnfjörð Bjarmason--- t/t5510-fetch.sh | 12 1 file changed, 12 insertions(+) diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh index fad65bd885..e5e88ee474 100755 --- a/t/t5510-fetch.sh +++ b/t/t5510-fetch.sh @@ -609,6 +609,10 @@ test_configured_prune () { test_configured_prune unset unset kept kept "" test_configured_prune unset unset kept kept "--no-prune" test_configured_prune unset unset pruned kept "--prune" +test_configured_prune unset unset kept pruned \ + "--prune origin +refs/tags/*:refs/tags/*" +test_configured_prune unset unset pruned pruned \ + "--prune origin +refs/tags/*:refs/tags/* +refs/heads/*:refs/remotes/origin/*" test_configured_prune false unset kept kept "" test_configured_prune false unset kept kept "--no-prune" @@ -625,6 +629,10 @@ test_configured_prune unset false pruned kept "--prune" test_configured_prune false false kept kept "" test_configured_prune false false kept kept "--no-prune" test_configured_prune false false pruned kept "--prune" +test_configured_prune false false kept pruned \ + "--prune origin +refs/tags/*:refs/tags/*" +test_configured_prune false false pruned pruned \ + "--prune origin +refs/tags/*:refs/tags/* +refs/heads/*:refs/remotes/origin/*" test_configured_prune true false kept kept "" test_configured_prune true false pruned kept "--prune" @@ -641,6 +649,10 @@ test_configured_prune false true pruned kept "--prune" test_configured_prune true true pruned kept "" test_configured_prune true true pruned kept "--prune" test_configured_prune true true kept kept "--no-prune" +test_configured_prune true true kept pruned \ + "--prune origin +refs/tags/*:refs/tags/*" +test_configured_prune true true pruned pruned \ + "--prune origin +refs/tags/*:refs/tags/* +refs/heads/*:refs/remotes/origin/*" test_expect_success 'all boundary commits are excluded' ' test_commit base && -- 2.15.1.424.g9478a66081
[PATCH v3 11/11] fetch: add a --fetch-prune option and fetch.pruneTags config
Add a --fetch-prune option to git-fetch, along with fetch.pruneTags config option. This allows for doing any of: git fetch -p -P git fetch --prune --prune-tags git fetch -p -P origin git fetch --prune --prune-tags origin Or simply: git config fetch.prune true && git config fetch.pruneTags true && git fetch Instead of the much more verbose: git fetch --prune origin '+refs/tags/*:refs/tags/*' '+refs/heads/*:refs/remotes/origin/*' Before this feature it was painful to support the use-case of pulling from a repo which is having both its branches *and* tags deleted regularly, and have our local references to reflect upstream. At work we create deployment tags in the repo for each rollout, and there's *lots* of those, so they're archived within weeks for performance reasons. Without this change it's hard to centrally configure such repos in /etc/gitconfig (on servers that are only used for working with them). You need to set fetch.prune=true globally, and then for each repo: git -C {} config --replace-all remote.origin.fetch "+refs/tags/*:refs/tags/*" "^\+*refs/tags/\*:refs/tags/\*$" Now I can simply set fetch.pruneTags=true in /etc/gitconfig as well, and users running "git pull" will automatically get the pruning semantics I want. Even though "git remote" has corresponding "prune" and "update --prune" subcommands I'm intentionally not adding a corresponding prune-tags or "update --prune --prune-tags" mode to that command. It's advertised (as noted in my recent "git remote doc: correct dangerous lies about what prune does") as only modifying remote tracking references, whereas any --prune-tags option is always going to modify what from the user's perspective is a local copy of the tag, since there's no such thing as a remote tracking tag. See my "Re: [BUG] git remote prune removes local tags, depending on fetch config" (87po6ahx87@evledraar.gmail.com; https://public-inbox.org/git/87po6ahx87@evledraar.gmail.com/) for more background info. Signed-off-by: Ævar Arnfjörð Bjarmason--- Documentation/config.txt | 14 ++ Documentation/fetch-options.txt| 15 ++- Documentation/git-fetch.txt| 27 +++ builtin/fetch.c| 32 contrib/completion/git-completion.bash | 2 +- remote.c | 5 - remote.h | 3 +++ t/t5510-fetch.sh | 30 ++ 8 files changed, 125 insertions(+), 3 deletions(-) diff --git a/Documentation/config.txt b/Documentation/config.txt index 0f27af5760..ae86455876 100644 --- a/Documentation/config.txt +++ b/Documentation/config.txt @@ -1401,6 +1401,14 @@ fetch.prune:: option was given on the command line. See also `remote..prune` and the PRUNING section of linkgit:git-fetch[1]. +fetch.pruneTags:: + If true, fetch will automatically behave as if the + `refs/tags/*:refs/tags/*` refspec was provided when pruning, + if not set already. This allows for setting both this option + and `fetch.prune` to maintain a 1=1 mapping to upstrem + refs. See also `remote..pruneTags` and the PRUNING + section of linkgit:git-fetch[1]. + fetch.output:: Control how ref update status is printed. Valid values are `full` and `compact`. Default value is `full`. See section @@ -2945,6 +2953,12 @@ remote..prune:: remove any remote-tracking references that no longer exist on the remote (as if the `--prune` option was given on the command line). Overrides `fetch.prune` settings, if any. + +remote..pruneTags:: + When set to true, fetching from this remote by default will also + remove any local tags that no longer exist on the remote if pruning + is activated in general via `remote..prune`, `fetch.prune` or + `--prune`. Overrides `fetch.pruneTags` settings, if any. + See also `remote..prune` and the PRUNING section of linkgit:git-fetch[1]. diff --git a/Documentation/fetch-options.txt b/Documentation/fetch-options.txt index 9f5c85ad96..dc13bed741 100644 --- a/Documentation/fetch-options.txt +++ b/Documentation/fetch-options.txt @@ -73,7 +73,20 @@ ifndef::git-pull[] are fetched due to an explicit refspec (either on the command line or in the remote configuration, for example if the remote was cloned with the --mirror option), then they are also - subject to pruning. + subject to pruning. Supplying `--prune-tags` is a shorthand for + providing the tag refspec. ++ +See the PRUNING section below for more details. + +-P:: +--prune-tags:: + Before fetching, remove any local tags that no longer exist on + the remote if `--prune` is enabled. This option should be used + more carefully, unlike `--prune` it will remove any local + references
[PATCH v3 10/11] fetch tests: add scaffolding for the new fetch.pruneTags
The fetch.pruneTags configuration doesn't exist yet, but will be added in a subsequent commit. Since testing for it requires adding new parameters to the test_configured_prune function it's easier to review this patch first to assert that no functional changes are introduced yet. Signed-off-by: Ævar Arnfjörð Bjarmason--- t/t5510-fetch.sh | 90 ++-- 1 file changed, 49 insertions(+), 41 deletions(-) diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh index e5e88ee474..840fd5ef02 100755 --- a/t/t5510-fetch.sh +++ b/t/t5510-fetch.sh @@ -551,18 +551,22 @@ set_config_tristate () { test_configured_prune () { fetch_prune=$1 remote_origin_prune=$2 - expected_branch=$3 - expected_tag=$4 - cmdline=$5 + fetch_prune_tags=$3 + remote_origin_prune_tags=$4 + expected_branch=$5 + expected_tag=$6 + cmdline=$7 - test_expect_success "prune fetch.prune=$1 remote.origin.prune=$2${5:+ $5}; branch:$3 tag:$4" ' + test_expect_success "prune fetch.prune=$1 remote.origin.prune=$2 fetch.pruneTags=$3 remote.origin.pruneTags=$4${7:+ $7}; branch:$5 tag:$6" ' # make sure a newbranch is there in . and also in one git branch -f newbranch && git tag -f newtag && ( cd one && test_unconfig fetch.prune && + test_unconfig fetch.pruneTags && test_unconfig remote.origin.prune && + test_unconfig remote.origin.pruneTags && git fetch && git rev-parse --verify refs/remotes/origin/newbranch && git rev-parse --verify refs/tags/newtag @@ -576,7 +580,9 @@ test_configured_prune () { ( cd one && set_config_tristate fetch.prune $fetch_prune && + set_config_tristate fetch.pruneTags $fetch_prune_tags && set_config_tristate remote.origin.prune $remote_origin_prune && + set_config_tristate remote.origin.pruneTags $remote_origin_prune_tags && git fetch $cmdline && case "$expected_branch" in @@ -601,57 +607,59 @@ test_configured_prune () { # $1 config: fetch.prune # $2 config: remote..prune -# $3 expect: branch to be pruned? -# $4 expect: tag to be pruned? -# $5 git-fetch $cmdline: +# $3 config: fetch.pruneTags +# $4 config: remote..pruneTags +# $5 expect: branch to be pruned? +# $6 expect: tag to be pruned? +# $7 git-fetch $cmdline: # -# $1$2$3 $4 $5 -test_configured_prune unset unset kept kept "" -test_configured_prune unset unset kept kept "--no-prune" -test_configured_prune unset unset pruned kept "--prune" -test_configured_prune unset unset kept pruned \ +# $1$2$3$4$5 $6 $7 +test_configured_prune unset unset unset unset kept kept "" +test_configured_prune unset unset unset unset kept kept "--no-prune" +test_configured_prune unset unset unset unset pruned kept "--prune" +test_configured_prune unset unset unset unset kept pruned \ "--prune origin +refs/tags/*:refs/tags/*" -test_configured_prune unset unset pruned pruned \ +test_configured_prune unset unset unset unset pruned pruned \ "--prune origin +refs/tags/*:refs/tags/* +refs/heads/*:refs/remotes/origin/*" -test_configured_prune false unset kept kept "" -test_configured_prune false unset kept kept "--no-prune" -test_configured_prune false unset pruned kept "--prune" +test_configured_prune false unset unset unset kept kept "" +test_configured_prune false unset unset unset kept kept "--no-prune" +test_configured_prune false unset unset unset pruned kept "--prune" -test_configured_prune true unset pruned kept "" -test_configured_prune true unset pruned kept "--prune" -test_configured_prune true unset kept kept "--no-prune" +test_configured_prune true unset unset unset pruned kept "" +test_configured_prune true unset unset unset pruned kept "--prune" +test_configured_prune true unset unset unset kept kept "--no-prune" -test_configured_prune unset false kept kept "" -test_configured_prune unset false kept kept "--no-prune" -test_configured_prune unset false pruned kept "--prune" +test_configured_prune unset false unset unset kept kept "" +test_configured_prune unset false unset unset kept kept "--no-prune" +test_configured_prune unset false unset unset pruned kept "--prune" -test_configured_prune false false kept kept "" -test_configured_prune false false kept kept "--no-prune" -test_configured_prune false false pruned kept "--prune" -test_configured_prune false false kept pruned \ +test_configured_prune false false unset unset
[PATCH v3 09/11] git-fetch & config doc: link to the new PRUNING section
Amend the documentation for fetch.prune, fetch..prune and --prune to link to the recently added PRUNING section. I'd have liked to link directly to it with "<>" from fetch-options.txt, since it's included in git-fetch.txt (git-pull.txt also includes it, but doesn't include that option). However making a reference across files yields this error: [...]/Documentation/git-fetch.xml:226: element xref: validity error : IDREF attribute linkend references an unknown ID "PRUNING" Signed-off-by: Ævar Arnfjörð Bjarmason--- Documentation/config.txt| 6 +- Documentation/fetch-options.txt | 3 +++ 2 files changed, 8 insertions(+), 1 deletion(-) diff --git a/Documentation/config.txt b/Documentation/config.txt index 0e25b2c92b..0f27af5760 100644 --- a/Documentation/config.txt +++ b/Documentation/config.txt @@ -1398,7 +1398,8 @@ fetch.unpackLimit:: fetch.prune:: If true, fetch will automatically behave as if the `--prune` - option was given on the command line. See also `remote..prune`. + option was given on the command line. See also `remote..prune` + and the PRUNING section of linkgit:git-fetch[1]. fetch.output:: Control how ref update status is printed. Valid values are @@ -2944,6 +2945,9 @@ remote..prune:: remove any remote-tracking references that no longer exist on the remote (as if the `--prune` option was given on the command line). Overrides `fetch.prune` settings, if any. ++ +See also `remote..prune` and the PRUNING section of +linkgit:git-fetch[1]. remotes.:: The list of remotes which are fetched by "git remote update diff --git a/Documentation/fetch-options.txt b/Documentation/fetch-options.txt index fb6bebbc61..9f5c85ad96 100644 --- a/Documentation/fetch-options.txt +++ b/Documentation/fetch-options.txt @@ -74,6 +74,9 @@ ifndef::git-pull[] line or in the remote configuration, for example if the remote was cloned with the --mirror option), then they are also subject to pruning. ++ +See the PRUNING section below for more details. + endif::git-pull[] ifndef::git-pull[] -- 2.15.1.424.g9478a66081
[PATCH v3 05/11] fetch tests: add a tag to be deleted to the pruning tests
Add a tag to be deleted to the fetch --prune tests. The tag is always kept for now, which is the expected behavior, but now I can add a test for tag pruning in a later commit. Signed-off-by: Ævar Arnfjörð Bjarmason--- t/t5510-fetch.sh | 93 1 file changed, 53 insertions(+), 40 deletions(-) diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh index ab8b25344d..fad65bd885 100755 --- a/t/t5510-fetch.sh +++ b/t/t5510-fetch.sh @@ -552,21 +552,25 @@ test_configured_prune () { fetch_prune=$1 remote_origin_prune=$2 expected_branch=$3 - cmdline=$4 + expected_tag=$4 + cmdline=$5 - test_expect_success "prune fetch.prune=$1 remote.origin.prune=$2${4:+ $4}; branch:$3" ' + test_expect_success "prune fetch.prune=$1 remote.origin.prune=$2${5:+ $5}; branch:$3 tag:$4" ' # make sure a newbranch is there in . and also in one git branch -f newbranch && + git tag -f newtag && ( cd one && test_unconfig fetch.prune && test_unconfig remote.origin.prune && git fetch && - git rev-parse --verify refs/remotes/origin/newbranch + git rev-parse --verify refs/remotes/origin/newbranch && + git rev-parse --verify refs/tags/newtag ) && # now remove it git branch -d newbranch && + git tag -d newtag && # then test ( @@ -582,6 +586,14 @@ test_configured_prune () { kept) git rev-parse --verify refs/remotes/origin/newbranch ;; + esac && + case "$expected_tag" in + pruned) + test_must_fail git rev-parse --verify refs/tags/newtag + ;; + kept) + git rev-parse --verify refs/tags/newtag + ;; esac ) ' @@ -590,44 +602,45 @@ test_configured_prune () { # $1 config: fetch.prune # $2 config: remote..prune # $3 expect: branch to be pruned? -# $4 git-fetch $cmdline: +# $4 expect: tag to be pruned? +# $5 git-fetch $cmdline: # -# $1$2$3 $4 -test_configured_prune unset unset kept "" -test_configured_prune unset unset kept "--no-prune" -test_configured_prune unset unset pruned "--prune" - -test_configured_prune false unset kept "" -test_configured_prune false unset kept "--no-prune" -test_configured_prune false unset pruned "--prune" - -test_configured_prune true unset pruned "" -test_configured_prune true unset pruned "--prune" -test_configured_prune true unset kept "--no-prune" - -test_configured_prune unset false kept "" -test_configured_prune unset false kept "--no-prune" -test_configured_prune unset false pruned "--prune" - -test_configured_prune false false kept "" -test_configured_prune false false kept "--no-prune" -test_configured_prune false false pruned "--prune" - -test_configured_prune true false kept "" -test_configured_prune true false pruned "--prune" -test_configured_prune true false kept "--no-prune" - -test_configured_prune unset true pruned "" -test_configured_prune unset true kept "--no-prune" -test_configured_prune unset true pruned "--prune" - -test_configured_prune false true pruned "" -test_configured_prune false true kept "--no-prune" -test_configured_prune false true pruned "--prune" - -test_configured_prune true true pruned "" -test_configured_prune true true pruned "--prune" -test_configured_prune true true kept "--no-prune" +# $1$2$3 $4 $5 +test_configured_prune unset unset kept kept "" +test_configured_prune unset unset kept kept "--no-prune" +test_configured_prune unset unset pruned kept "--prune" + +test_configured_prune false unset kept kept "" +test_configured_prune false unset kept kept "--no-prune" +test_configured_prune false unset pruned kept "--prune" + +test_configured_prune true unset pruned kept "" +test_configured_prune true unset pruned kept "--prune" +test_configured_prune true unset kept kept "--no-prune" + +test_configured_prune unset false kept kept "" +test_configured_prune unset false kept kept "--no-prune" +test_configured_prune unset false pruned kept "--prune" + +test_configured_prune false false kept kept "" +test_configured_prune false false kept kept "--no-prune" +test_configured_prune false false pruned kept "--prune" + +test_configured_prune true false kept kept "" +test_configured_prune true false pruned kept "--prune" +test_configured_prune
[PATCH v3 07/11] git fetch doc: add a new section to explain the ins & outs of pruning
Add a new section to canonically explain how remote reference pruning works, and how users should be careful about using it in conjunction with tag refspecs in particular. A subsequent commit will update the git-remote documentation to refer to this section, and details the motivation for writing this in the first place. Signed-off-by: Ævar Arnfjörð Bjarmason--- Documentation/git-fetch.txt | 49 + 1 file changed, 49 insertions(+) diff --git a/Documentation/git-fetch.txt b/Documentation/git-fetch.txt index b153aefa68..18fac0da2e 100644 --- a/Documentation/git-fetch.txt +++ b/Documentation/git-fetch.txt @@ -99,6 +99,55 @@ The latter use of the `remote..fetch` values can be overridden by giving the `--refmap=` parameter(s) on the command line. +PRUNING +--- + +Git has a default disposition of keeping data unless it's explicitly +thrown away; this extends to holding onto local references to branches +on remotes that have themselves deleted those branches. + +If left to accumulate, these stale references might make performance +worse on big and busy repos that have a lot of branch churn, and +e.g. make the output of commands like `git branch -a --contains +` needlessly verbose, as well as impacting anything else +that'll work with the complete set of known references. + +These remote tracking references can be deleted as a one-off with +either of: + + +# While fetching +$ git fetch --prune + +# Only prune, don't fetch +$ git remote prune + + +To prune references as part of your normal workflow without needing to +remember to run that set `fetch.prune` globally, or +`remote..prune` per-remote in the config. See +linkgit:git-config[1]. + +Here's where things get tricky and more specific. The pruning feature +doesn't actually care about branches, instead it'll prune local <-> +remote references as a function of the refspec of the remote (see +`` and < > above). + +Therefore if the refspec for the remote includes +e.g. `refs/tags/*:refs/tags/*`, or you manually run e.g. `git fetch +--prune "refs/tags/*:refs/tags/*"` it won't be stale remote +tracking branches that are deleted, but any local tag that doesn't +exist on the remote. + +This might not be what you expect, i.e. you want to prune remote +``, but also explicitly fetch tags from it, so when you fetch +from it you delete all your local tags, most of which may not have +come from the `` remote in the first place. + +So be careful when using this with a refspec like +`refs/tags/*:refs/tags/*`, or any other refspec which might map +references from multiple remotes to the same local namespace. + OUTPUT -- -- 2.15.1.424.g9478a66081
[PATCH v3 02/11] fetch: stop accessing "remote" variable indirectly
Access the "remote" variable passed to the fetch_one() directly rather than through the gtransport wrapper struct constructed in this function for other purposes. This makes the code more readable, as it's now obvious that the remote struct doesn't somehow get munged by the prepare_transport() function above, which takes the "remote" struct as an argument and constructs the "gtransport" struct, containing among other things the "remote" struct. A subsequent change will copy this pattern to access a new remote->prune-tags field, but without hte use of the gtransport variable. It's useful once that change lands to see that the two pieces of code behave exactly the same. This pattern of accessing the container struct was added in 737c5a9cde ("fetch: make --prune configurable", 2013-07-13) when this code was initially introduced. Signed-off-by: Ævar Arnfjörð Bjarmason--- builtin/fetch.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/builtin/fetch.c b/builtin/fetch.c index b34665db9e..a85c2002a9 100644 --- a/builtin/fetch.c +++ b/builtin/fetch.c @@ -1280,8 +1280,8 @@ static int fetch_one(struct remote *remote, int argc, const char **argv) if (prune < 0) { /* no command line request */ - if (0 <= gtransport->remote->prune) - prune = gtransport->remote->prune; + if (0 <= remote->prune) + prune = remote->prune; else if (0 <= fetch_prune_config) prune = fetch_prune_config; else -- 2.15.1.424.g9478a66081
[PATCH v3 03/11] fetch tests: refactor in preparation for testing tag pruning
In a subsequent commit this function will learn to test for tag pruning, prepare for that by making space for more variables, and making it clear that "expected" here refers to branches. Signed-off-by: Ævar Arnfjörð Bjarmason--- t/t5510-fetch.sh | 9 ++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh index 668c54be41..11da97f9b7 100755 --- a/t/t5510-fetch.sh +++ b/t/t5510-fetch.sh @@ -549,9 +549,12 @@ set_config_tristate () { } test_configured_prune () { - fetch_prune=$1 remote_origin_prune=$2 cmdline=$3 expected=$4 + fetch_prune=$1 + remote_origin_prune=$2 + cmdline=$3 + expected_branch=$4 - test_expect_success "prune fetch.prune=$1 remote.origin.prune=$2${3:+ $3}; $4" ' + test_expect_success "prune fetch.prune=$1 remote.origin.prune=$2${3:+ $3}; branch:$4" ' # make sure a newbranch is there in . and also in one git branch -f newbranch && ( @@ -572,7 +575,7 @@ test_configured_prune () { set_config_tristate remote.origin.prune $remote_origin_prune && git fetch $cmdline && - case "$expected" in + case "$expected_branch" in pruned) test_must_fail git rev-parse --verify refs/remotes/origin/newbranch ;; -- 2.15.1.424.g9478a66081
Re: [PATCH] daemon: add --no-syslog to undo implicit --syslog
Thanks for your responses! On 23.01.2018 01:00, Ævar Arnfjörð Bjarmason wrote: > This patch looks good, but I wonder if with the rise of systemd > there's a good reason to flip the default around to not having other > stuff imply --syslog, and have users specify this implictly, then we > won't need a --no-syslog option. > > But maybe that'll break too much stuff. > That seems risky to me – even with systemd, the StandardError directive by default inherits StandardOutput, so if you set StandardOutput=socket without StandardError=journal, log output in stderr clobbers regular output. (Also, stderr is apparently closed with --detach, see below.) On 23.01.2018 19:30, Junio C Hamano wrote: > Ævar Arnfjörð Bjarmasonwrites: > >> On Mon, Jan 22 2018, Lucas Werkmeister jotted: >> >>> Several options imply --syslog, without there being a way to >>> disable it again. This commit adds that option. >> >> Just two options imply --syslog, --detach & --inetd, unless I've >> missed something, anyway 2 != several, so maybe just say "The >> --detach and --inetd options imply --syslog ...". > > Correct. I respectfully disagree on “2 != several”, but sure, I can repeat the two options in the message instead :) > Moreover, --detach completely dissociates the process from the > original set of standard file descriptors by first closing them and > then connecting it to "/dev/null", so it will be nonsense to use this > new option with it. > Ah, I wasn’t aware of that – so with --detach, --no-syslog would be better described as “disables all logging” rather than “log to stderr instead”. IMHO it would still make sense to have the --no-syslog option (then mainly for use with --inetd) as long as its interaction with --inetd is properly documented… do you agree? If yes, I’ll be glad to submit another version of the patch. Best regards, Lucas smime.p7s Description: S/MIME Cryptographic Signature
HELLO
-- Hello Dear Friend, Greetings and how are you doing? I want to know if you are keen to be my partner in claiming the fortune $12.8 Million USD left by a late client. If you're interested revert for more details. Awaiting your reply Hamza Kabore
Re: [PATCH v4 5/6] convert: add 'working-tree-encoding' attribute
Jeff Kingwrites: > But with Coccinelle, it's a lot easier to apply the change tree-wide, and > to convert topics in flight as they get merged. The maintainer still > gets conflicts with topics-in-flight that touch converted areas, though. > So I'd be curious to hear if Junio's opinion has changed at all. There are two distinct kinds of cost on such a tree-wide change. Conflicts with in-flight topic cannot be avoided other than truly avoiding, i.e. refraining from touching the areas in flux, but it is primarily what the maintainer does, and with help with rerere it can be reasonably well automated ;-) But the cost of reviewing could become a lot smaller when our tools are trustworthy. As long as we can be reasonably certain that the tree-wide change patch does one thing it is intended to do and nothing else (e.g. comes with mechanical reproduction recipe that allows the patch to be independently audited), I do not have much problem with such a clean-up. The "avoid tree-wide change" rule still applies for things that allows a lot of subjective judgment and discretion. I do not know of a good way to reduce reviewer costs on those kind of changes. Thanks.
Re: [PATCH v4 0/6] convert: add support for different encodings
Torsten Bögershausenwrites: > On Sat, Jan 20, 2018 at 04:24:12PM +0100, lars.schnei...@autodesk.com wrote: >> From: Lars Schneider >> >> Hi, >> >> Patches 1-4 and 6 are preparation and helper functions. >> Patch 5 is the actual change. > > I (still) have 2 remarks on convert.c - to make live easier, > I will send a small "on top" patch the next days. Thanks, both. I'll stay on the sideline ;-) and deal with other topics first.
RE: [PATCH v2 0/6] Force pipes to flush immediately on NonStop platform
On January 23, 2018 1:13 PM, Junio C Hamano wrote: > "Randall S. Becker"writes: > > >> IOW, I do not see it explained clearly why this change is needed on > >> any single platform---so "that issue may be shared by others, too" > >> is a bit premature thing for me to listen to and understand, as "that > >> issue" is quite unclear to me. > > > > v4 might be a little better. The issue seems to be specific to NonStop > > that it's PIPE mechanism needs to have setbuf(pipe,NULL) called for > > git to be happy. The default behaviour appears to be different on > > NonStop from other platforms from our testing. We get hung up waiting > > on pipes unless this is done. > > I am afraid that that is not a "diagnosis" enough to allow us moving forward. > We get hung up because...? When the process that has the other end of > pipe open exits, NonStop does not close the pipe properly? Or NonStop > does not flush the data buffered in the pipe? > Would it help if a compat wrapper that does setbuf(fd, NULL) immediately > before closing the fd, or some other more targetted mechanism, is used only > on NonStop, for example? Potentially megabytes of data can pass thru a > pipe, and if the platform bug affects only at the tail end of the transfer, > marking the pipe not to buffer at all at the beginning is too big a hammer to > work it around. With the explanation given so far, this still smells more > like > "we have futzed around without understanding why, and this happens to > work." It may be good enough for your purpose of making progress (after > all, I'd imagine that you'd need to work this around one way or another to > hunt for and fix more issues on the platform), but it does not sound like "we > know what the problem is, and this is the best workaround for that", which is > what we want if it wants to become part of the official codebase. I am retesting without setbuf(NULL) but this is unlikely to be enlightening. The test cases do not adequately simulate the configuration in which my team originally encountered the problem. This requires a guarantee of the source and destination coming through different logical CPUs. We never encountered the issue in the test suite, only when end users got hold of it. We had two distinct problems, one which was the revent=0 related hang (already solved) and other was a stream flush problem. The two are related but distinct. The platform support group insisted that we should have the setbuf(NULL) in place for interprocess communications in the form used here - I'm worried about losing this, but completely aware that this is far too heavy for other platforms (hence the __TANDEM guard in wrapper.c). If the form of the wrapper should be different, I would be happy to comply. I have a much longer explanation about the platform message stack structure, but that doesn't belong here. Happy to respond privately if requested. Cheers, Randall -- Brief whoami: NonStop developer since approximately 2112884442 UNIX developer since approximately 421664400 -- In my real life, I talk too much.
Re: [PATCH 0/8] rebase -i: offer to recreate merge commits
Johannes Schindelinwrites: > My original attempt was --preserve-merges, but that design was so > limited that I did not even enable it in interactive mode. > ... > There are more patches in the pipeline, based on this patch series, but > left for later in the interest of reviewable patch series: one mini > series to use the sequencer even for `git rebase -i --root`, and another > one to add support for octopus merges to --recreate-merges. I left comments on a handful of them, but I do not think any of them spotted a grave design issue to be a show stopper. Overall, the series was quite a pleasant read, even with those minor nits and rooms for improvements. Thanks.
Re: [PATCH 6/8] sequencer: handle autosquash and post-rewrite for merge commands
Jacob Kellerwrites: >> static int is_per_worktree_ref(const char *refname) >> { >> return !strcmp(refname, "HEAD") || >> - starts_with(refname, "refs/bisect/"); >> + starts_with(refname, "refs/bisect/") || >> + starts_with(refname, "refs/rewritten/"); >> } > > Would this part make more sense to move into the commit that > introduces writing these refs, or does it only matter once you start > this step here? Good spotting. I too was wondering about multiple worktrees when I saw the "label" thing introduced. It probably makes sense to move this to that step.
Re: [PATCH 5/8] rebase: introduce the --recreate-merges option
Johannes Schindelinwrites: > diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt > index 8a861c1e0d6..1d061373288 100644 > --- a/Documentation/git-rebase.txt > +++ b/Documentation/git-rebase.txt > @@ -368,6 +368,11 @@ The commit list format can be changed by setting the > configuration option > rebase.instructionFormat. A customized instruction format will automatically > have the long commit hash prepended to the format. > > +--recreate-merges:: > + Recreate merge commits instead of flattening the history by replaying > + merges. Merge conflict resolutions or manual amendments to merge > + commits are not preserved. > + It is sensible to postpone tackling "evil merges" in this initial iteration of the series, and "manual amendments ... not preserved" is a reasonable thing to document. But do we want to say a bit more about conflicting merges? "conflict resolutions ... not preserved" sounds as if it does not stop and instead record the result with conflict markers without even letting rerere to kick in, which certainly is not the impression you wanted to give to the readers. I am imagining that it will stop and give control back to the end user just like a conflicted "pick" would, and allow "rebase --continue" to record resolution from the working tree, and just like conflicted "pick", it would allow rerere() to help end users recall previous resolution.
Re: [PATCH v4 0/6] convert: add support for different encodings
On Sat, Jan 20, 2018 at 04:24:12PM +0100, lars.schnei...@autodesk.com wrote: > From: Lars Schneider> > Hi, > > Patches 1-4 and 6 are preparation and helper functions. > Patch 5 is the actual change. I (still) have 2 remarks on convert.c - to make live easier, I will send a small "on top" patch the next days.
Re: [PATCH 4/8] rebase-helper --make-script: introduce a flag to recreate merges
Eric Sunshinewrites: >> + is_octopus = to_merge && to_merge->next; >> + >> + if (is_octopus) >> + BUG("Octopus merges not yet supported"); > > Is this a situation which the end-user can trigger by specifying a > merge with more than two parents? If so, shouldn't this be just a > normal error message rather than a (developer) bug message? Or, am I > misunderstanding? BUG() is "we wrote code carefully so that this should not trigger; we do not _expect_ the code to reach here". This one is expected to trigger, and I agree with you that it should be die(), if the series is meant to be released to the general public in the current form (i.e. until the limitation is lifted so that it can handle an octopus). If the callers are made more careful to check if there is an octopus involved and reject the request early, then seeing an octopus in this location in a loop will become a BUG().
Re: [PATCH 4/8] rebase-helper --make-script: introduce a flag to recreate merges
Johannes Schindelinwrites: > The sequencer just learned a new commands intended to recreate branch s/a //; > structure (similar in spirit to --preserve-merges, but with a > substantially less-broken design). > ... > @@ -2785,6 +2787,335 @@ void append_signoff(struct strbuf *msgbuf, int > ignore_footer, unsigned flag) > strbuf_release(); > } > > +struct labels_entry { > + struct hashmap_entry entry; > + char label[FLEX_ARRAY]; > +}; > + > +static int labels_cmp(const void *fndata, const struct labels_entry *a, > + const struct labels_entry *b, const void *key) > +{ > + return key ? strcmp(a->label, key) : strcmp(a->label, b->label); > +} label_oid() accesses state->labels hash using strihash() as the hash function, but the final comparison between the entries in the same hash buckets are done with case sensitivity. It is unclear to me if that is what was intended, and why. > +struct string_entry { > + struct oidmap_entry entry; > + char string[FLEX_ARRAY]; > +}; > + > +struct label_state { > + struct oidmap commit2label; > + struct hashmap labels; > + struct strbuf buf; > +}; > + > +static const char *label_oid(struct object_id *oid, const char *label, > + struct label_state *state) > +{ > + struct labels_entry *labels_entry; > + struct string_entry *string_entry; > + struct object_id dummy; > + size_t len; > + int i; > + > + string_entry = oidmap_get(>commit2label, oid); > + if (string_entry) > + return string_entry->string; > + > + /* > + * For "uninteresting" commits, i.e. commits that are not to be > + * rebased, and which can therefore not be labeled, we use a unique > + * abbreviation of the commit name. This is slightly more complicated > + * than calling find_unique_abbrev() because we also need to make > + * sure that the abbreviation does not conflict with any other > + * label. > + * > + * We disallow "interesting" commits to be labeled by a string that > + * is a valid full-length hash, to ensure that we always can find an > + * abbreviation for any uninteresting commit's names that does not > + * clash with any other label. > + */ > + if (!label) { > + char *p; > + > + strbuf_reset(>buf); > + strbuf_grow(>buf, GIT_SHA1_HEXSZ); > + label = p = state->buf.buf; > + > + find_unique_abbrev_r(p, oid->hash, default_abbrev); > + > + /* > + * We may need to extend the abbreviated hash so that there is > + * no conflicting label. > + */ > + if (hashmap_get_from_hash(>labels, strihash(p), p)) { > + size_t i = strlen(p) + 1; > + > + oid_to_hex_r(p, oid); > + for (; i < GIT_SHA1_HEXSZ; i++) { > + char save = p[i]; > + p[i] = '\0'; > + if (!hashmap_get_from_hash(>labels, > +strihash(p), p)) > + break; > + p[i] = save; > + } > + } If oid->hash required full 40-hex to disambiguate, then find-unique-abbrev would give 40-hex and we'd want the same "-" suffix technique employed below to make it consistently unique. I wonder if organizing the function this way ... if (!label) label = oid-to-hex(oid); if (label already exists or full oid) { make it unambiguous; } ... allows the resulting code easier to understand and manage. A related tangent. Does an auto-label given to "uninteresting" commit need to be visible to end users? I doubted it and that is why I said oid-to-hex in the above, but if it is given to end users, use of find-unique-abbrev-r is perfectly fine. > +static int make_script_with_merges(struct pretty_print_context *pp, > +struct rev_info *revs, FILE *out, > +unsigned flags) > +{ > + ... > + int abbr = flags & TODO_LIST_ABBREVIATE_CMDS; > + const char *p = abbr ? "p" : "pick", *l = abbr ? "l" : "label", > + *t = abbr ? "t" : "reset", *b = abbr ? "b" : "bud", > + *m = abbr ? "m" : "merge"; It would be easier to understand if these short named variables are reserved only for temporary use, not as constants. It is not too much to spell fprintf(out, "%s onto\n", cmd_label); than fprintf(out, "%s onto\n", l); and would save readers from head-scratching, wondering where the last assignment to variable "l" is. > + > + oidmap_init(, 0); > + oidmap_init(, 0); > + hashmap_init(, (hashmap_cmp_fn) labels_cmp, NULL, 0); > + strbuf_init(, 32); > + > + if (revs->cmdline.nr &&
Re: [PATCH 3/8] sequencer: fast-forward merge commits, if possible
Phillip Woodwrites: > On 18/01/18 15:35, Johannes Schindelin wrote: >> >> Just like with regular `pick` commands, if we are trying to recreate a >> merge commit, we now test whether the parents of said commit match HEAD >> and the commits to be merged, and fast-forward if possible. >> >> This is not only faster, but also avoids unnecessary proliferation of >> new objects. > > I might have missed something but shouldn't this be checking opts->allow_ff? Because the whole point of this mechanism is to recreate the topology faithfully to the original, even if the original was a redundant merge (which has a side parent that is an ancestor or a descendant of the first parent), we should just point at the original merge when the condition allows it, regardless of opts->allow_ff. I think it is a different matter if an insn to create a new merge (i.e. "merge - ", not "merge ") should honor opts->allow_ff; because it is not about recreating an existing history but is a way to create what did not exist before, I think it is sensible if allow_ff option is honored.
Re: [PATCH 3/8] sequencer: fast-forward merge commits, if possible
Johannes Schindelinwrites: > + /* > + * If HEAD is not identical to the parent of the original merge commit, > + * we cannot fast-forward. > + */ > + can_fast_forward = commit && commit->parents && > + !oidcmp(>parents->item->object.oid, > + _commit->object.oid); > + I think this expression and assignment should better be done much later. Are you going to update commit, commit->parents, etc. that are involved in the computation in the meantime??? > strbuf_addf(_name, "refs/rewritten/%.*s", merge_arg_len, arg); > merge_commit = lookup_commit_reference_by_name(ref_name.buf); > if (!merge_commit) { > @@ -2164,6 +2172,17 @@ static int do_merge(struct commit *commit, const char > *arg, int arg_len, > rollback_lock_file(); > return -1; > } > + > + if (can_fast_forward && commit->parents->next && > + !commit->parents->next->next && > + !oidcmp(>parents->next->item->object.oid, > + _commit->object.oid)) { ... Namely, here. Because the earlier one is computing "are we replaying exactly the same commit on top of exactly the same state?", which is merely one half of "can we fast-forward", and storing it in a variable whose name is over-promising way before it becomes necessary. The other half of "can we fast-forward?" logic is the remainder of the if() condition we see above. IOW, when fully spelled, this code can fast-forward when we are replaying a commit on top of exactly the same first-parent and the commit being replayed is a single parent merge. We may even want to get rid of can_fast_forward variable. > + strbuf_release(_name); > + rollback_lock_file(); > + return fast_forward_to(>object.oid, > +_commit->object.oid, 0, opts); > + } > + > write_message(oid_to_hex(_commit->object.oid), GIT_SHA1_HEXSZ, > git_path_merge_head(), 0); > write_message("no-ff", 5, git_path_merge_mode(), 0);
Re: [PATCH v2 1/2] t7505: Add tests for cherry-pick and rebase -i/-p
Phillip Woodwrites: > + export GIT_SEQUENCE_EDITOR GIT_EDITOR && > + test_must_fail git rebase $mode b && > + echo x>a && "echo x >a"
Re: [PATCH v2 1/2] t7505: Add tests for cherry-pick and rebase -i/-p
Phillip Woodwrites: > @@ -31,17 +63,40 @@ mkdir -p "$HOOKDIR" > echo "#!$SHELL_PATH" > "$HOOK" > cat >> "$HOOK" <<'EOF' > > +GIT_DIR=$(git rev-parse --git-dir) > +if test -d "$GIT_DIR/rebase-merge" > +then > + rebasing=1 > +else > + rebasing=0 > +fi > + > +get_last_cmd () { > + tail -n1 "$GIT_DIR/rebase-merge/done" | { > +read cmd id _ > +git log --pretty="[$cmd %s]" -n1 $id > + } > +} > + > if test "$2" = commit; then > - source=$(git rev-parse "$3") > + if test $rebasing = 1 > + then > +source="$3" > + else > +source=$(git rev-parse "$3") > + fi > else >source=${2-default} > fi > -if test "$GIT_EDITOR" = :; then > - sed -e "1s/.*/$source (no editor)/" "$1" > msg.tmp > +test "$GIT_EDITOR" = : && source="$source (no editor)" > + > +if test $rebasing = 1 > +then > + echo "$source $(get_last_cmd)" >"$1" > else >sed -e "1s/.*/$source/" "$1" > msg.tmp > + mv msg.tmp "$1" > fi It is somewhat irritating that indentation is screwed up in this part of the file. Can we not make it even worse?
Re: The original file that was split in 2 other files, is there a way in git to see what went where?
Jeff Kingwrites: > On Mon, Jan 22, 2018 at 10:22:21PM -0500, Aleksey Bykov wrote: > >> I am a code reviewer, I have a situation in GIT: >> >> - before: a.txt >> >> Then a developer decided to split the content of a.txt into 2 files >> and add a few changes all in one commit: >> >> - after: b.txt + few changes and c.txt + few changes >> ... > For seeing which line came from where, you might try "git blame -C", > which will cross file boundaries looking for the source of lines. > ... > And finally, if you're going to do a lot with "git blame", I'd look into > the "tig" tool as a prettier interface. You should be able to do "tig > blame -C ..." in the same way. All excellent guides. "blame" is good at explaining where things came from, but not as good at explaining, starting from an old state, where things went. "blame --reverse" does a decent job within the constraints its output format has, but not quite ideal.
Re: [PATCH] Fixes compile warning with -Wimplicit-fallthrough CFLAGS
On Tue, Jan 23, 2018 at 10:33:57AM -0800, Junio C Hamano wrote: > Jeff Kingwrites: > > >> diff --git a/apply.c b/apply.c > >> index 321a9fa68..a22fb2881 100644 > >> --- a/apply.c > >> +++ b/apply.c > >> @@ -1450,7 +1450,7 @@ static void recount_diff(const char *line, int size, > >> struct fragment *fragment) > >>switch (*line) { > >>case ' ': case '\n': > >>newlines++; > >> - /* fall through */ > >> + GIT_FALLTHROUGH; > > > > Ugh, the semi-colon there makes it look like it's actual code. If we go > > this route, I wonder if it's worth hiding it inside the macro. > > What? You mean to shout in all caps without even terminating the > line with any punctuation? Please don't---I am sure it will break > auto indentation people rely on from their editors. True, that may be even worse. I just wonder if we can do something to make it look more obviously like a non-code attribute. The actual syntax is something like: [[fallthrough]]; which is pretty horrid, but at least a bit easier to see. gcc also provides "__attribute__((fallthrough))", but I don't think it works with clang. I vastly prefer the comment approach if we can use it. Apparently clang doesn't support it, but I have also not managed to get clang (either version 4, 6, or the upcoming 7) to actually report anything via -Wimplicit-fallthrough, either. Maybe I'm holding it wrong. -Peff
Re: [PATCH] Fixes compile warning with -Wimplicit-fallthrough CFLAGS
Jeff Kingwrites: >> diff --git a/apply.c b/apply.c >> index 321a9fa68..a22fb2881 100644 >> --- a/apply.c >> +++ b/apply.c >> @@ -1450,7 +1450,7 @@ static void recount_diff(const char *line, int size, >> struct fragment *fragment) >> switch (*line) { >> case ' ': case '\n': >> newlines++; >> -/* fall through */ >> +GIT_FALLTHROUGH; > > Ugh, the semi-colon there makes it look like it's actual code. If we go > this route, I wonder if it's worth hiding it inside the macro. What? You mean to shout in all caps without even terminating the line with any punctuation? Please don't---I am sure it will break auto indentation people rely on from their editors.
Re: [PATCH] daemon: add --no-syslog to undo implicit --syslog
Ævar Arnfjörð Bjarmasonwrites: > On Mon, Jan 22 2018, Lucas Werkmeister jotted: > >> Several options imply --syslog, without there being a way to disable it >> again. This commit adds that option. > > Just two options imply --syslog, --detach & --inetd, unless I've missed > something, anyway 2 != several, so maybe just say "The --detach and > --inetd options imply --syslog ...". Correct. Moreover, --detach completely dissociates the process from the original set of standard file descriptors by first closing them and then connecting it to "/dev/null", so it will be nonsense to use this new option with it.
Re: [PATCH] sequencer: mark a file local symbol as static
Ramsay Joneswrites: > Signed-off-by: Ramsay Jones > --- > > Hi Phillip, > > If you need to re-roll your 'pw/sequencer-in-process-commit' branch, could > you please squash this into the relevant patch (commit da96adcf5a, > "sequencer: run 'prepare-commit-msg' hook", 2018-01-19). > > Thanks. Thanks; as the commit is sitting at the tip of the topic not yet in 'next', let me just squash it in. > > ATB, > Ramsay Jones > > sequencer.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/sequencer.c b/sequencer.c > index 79579ba11..5bfdc4044 100644 > --- a/sequencer.c > +++ b/sequencer.c > @@ -893,7 +893,7 @@ void commit_post_rewrite(const struct commit *old_head, > run_rewrite_hook(_head->object.oid, new_head); > } > > -int run_prepare_commit_msg_hook(struct strbuf *msg, const char *commit) > +static int run_prepare_commit_msg_hook(struct strbuf *msg, const char > *commit) > { > struct argv_array hook_env = ARGV_ARRAY_INIT; > int ret;
Re: [PATCH] fsck: fix leak when traversing trees
Eric Wongwrites: > While fsck_walk/fsck_walk_tree/parse_tree populates "struct tree" > idempotently, it is still up to the fsck_walk caller to call > free_tree_buffer. > > Fixes: ad2db4030e42890e ("fsck: remove redundant parse_tree() invocation") Yup, we can see that that commit did stop freeing the tree buffer. Thanks. > > Signed-off-by: Eric Wong > --- > These APIs could probably be made to be less error-prone, > but at least this stops my little machine from OOM-ing. > > builtin/fsck.c | 8 +++- > 1 file changed, 7 insertions(+), 1 deletion(-) > > diff --git a/builtin/fsck.c b/builtin/fsck.c > index 04846d46f9..92ce775a74 100644 > --- a/builtin/fsck.c > +++ b/builtin/fsck.c > @@ -171,7 +171,13 @@ static void mark_object_reachable(struct object *obj) > > static int traverse_one_object(struct object *obj) > { > - return fsck_walk(obj, obj, _walk_options); > + int result = fsck_walk(obj, obj, _walk_options); > + > + if (obj->type == OBJ_TREE) { > + struct tree *tree = (struct tree *)obj; > + free_tree_buffer(tree); > + } > + return result; > } > > static int traverse_reachable(void)
Re: [PATCH v2 0/6] Force pipes to flush immediately on NonStop platform
"Randall S. Becker"writes: >> IOW, I do not see it explained clearly why this change is needed on any >> single >> platform---so "that issue may be shared by others, too" >> is a bit premature thing for me to listen to and understand, as "that issue" >> is >> quite unclear to me. > > v4 might be a little better. The issue seems to be specific to > NonStop that it's PIPE mechanism needs to have setbuf(pipe,NULL) > called for git to be happy. The default behaviour appears to be > different on NonStop from other platforms from our testing. We get > hung up waiting on pipes unless this is done. I am afraid that that is not a "diagnosis" enough to allow us moving forward. We get hung up because...? When the process that has the other end of pipe open exits, NonStop does not close the pipe properly? Or NonStop does not flush the data buffered in the pipe? Would it help if a compat wrapper that does setbuf(fd, NULL) immediately before closing the fd, or some other more targetted mechanism, is used only on NonStop, for example? Potentially megabytes of data can pass thru a pipe, and if the platform bug affects only at the tail end of the transfer, marking the pipe not to buffer at all at the beginning is too big a hammer to work it around. With the explanation given so far, this still smells more like "we have futzed around without understanding why, and this happens to work." It may be good enough for your purpose of making progress (after all, I'd imagine that you'd need to work this around one way or another to hunt for and fix more issues on the platform), but it does not sound like "we know what the problem is, and this is the best workaround for that", which is what we want if it wants to become part of the official codebase.
Re: The original file that was split in 2 other files, is there a way in git to see what went where?
On Mon, Jan 22, 2018 at 10:22:21PM -0500, Aleksey Bykov wrote: > I am a code reviewer, I have a situation in GIT: > > - before: a.txt > > Then a developer decided to split the content of a.txt into 2 files > and add a few changes all in one commit: > > - after: b.txt + few changes and c.txt + few changes > > Is there an easy way to see: > > 1. what came to b from a? > 2 .what came to c from a? > 3. all extra changes apart from just moving stuff? Jonathan suggested the new "--color-moved", which I second as a good way of seeing what was moved, and which lines were changed. For seeing which line came from where, you might try "git blame -C", which will cross file boundaries looking for the source of lines. For instance, here's a case in git where some code was moved: git blame -C ae563542bf10fa8c33abd2a354e4b28aca4264d7 revision.c You can see which lines are new to the file, and which ones were moved from elsewhere. If you want to simplify the "noise" of seeing the actual origin of each line, you can ask blame not to go further back. Like: commit=ae563542bf10fa8c33abd2a354e4b28aca4264d7 git blame -b -C $commit^..$commit revision.c That will leave the commit id blank for every line that wasn't touched as part of the commit (or if you had a whole series of commits, replace "$commit^" with the parent of the series). And finally, if you're going to do a lot with "git blame", I'd look into the "tig" tool as a prettier interface. You should be able to do "tig blame -C ..." in the same way. -Peff
Re: [PATCH 5/5] travis-ci: don't fail if user already exists on 32 bit Linux build job
On Mon, Jan 22, 2018 at 02:32:20PM +0100, SZEDER Gábor wrote: > The 32 bit Linux build job runs in a Docker container, which lends > itself to running and debugging locally, too. Especially during > debugging one usually doesn't want to start with a fresh container > every time, to save time spent on installing a bunch of dependencies. > However, that doesn't work quite smootly, because the script running > in the container always creates a new user, which then must be removed > every time before subsequent executions, or the build script fails. > > Make this process more convenient and don't try to create that user if > it already exists and has the right user ID in the container, so > developers don't have to bother with running a 'userdel' each time > before they run the build script. Makes sense. > The build job on Travis CI always starts with a fresh Docker > container, so this change doesn't make a difference there. I wonder if that is fixable. Installing dependencies into the container takes quite a lot of time, and is just wasted effort. > diff --git a/ci/run-linux32-build.sh b/ci/run-linux32-build.sh > index e37e1d2d5f..13047adde3 100755 > --- a/ci/run-linux32-build.sh > +++ b/ci/run-linux32-build.sh > @@ -33,7 +33,13 @@ then > CI_USER=root > else > CI_USER=ci > - useradd -u $HOST_UID $CI_USER > + if test "$(id -u $CI_USER)" = $HOST_UID > + then > + : # user already exists with the right ID > + else > + useradd -u $HOST_UID $CI_USER > + fi Is it worth redirecting the stderr of "id" to avoid noise when $CI_USER does not yet exist at all? Or is that a useful log message? :) -Peff
Re: [PATCH 4/5] travis-ci: don't run the test suite as root in the 32 bit Linux build
On Mon, Jan 22, 2018 at 02:32:19PM +0100, SZEDER Gábor wrote: > Travis CI runs the 32 bit Linux build job in a Docker container, where > all commands are executed as root by default. Therefore, ever since > we added this build job in 88dedd5e7 (Travis: also test on 32-bit > Linux, 2017-03-05), we have a bit of code to create a user in the > container matching the ID of the host user and then to run the test > suite as this user. Matching the host user ID is important, because > otherwise the host user would have no access to any files written by > processes running in the container, notably the logs of failed tests > couldn't be included in the build job's trace log. > > Alas, this piece of code never worked, because it sets the variable > holding the user name ($CI_USER) in a subshell, meaning it doesn't > have any effect by the time we get to the point to actually use the > variable to switch users with 'su'. So all this time we were running > the test suite as root. This all makes sense to me, and the patch looks sane. > Reorganize that piece of code in 'ci/run-linux32-build.sh' a bit to > avoid that problematic subshell and to ensure that we switch to the > right user. Furthermore, make the script's optional host user ID > option mandatory, so running the build accidentally as root will > become harder when debugging locally. If someone really wants to run > the test suite as root, whatever the reasons might be, it'll still be > possible to do so by explicitly passing '0' as host user ID. Coincidentally, we recently set up a docker test build for our local fork of Git. We found the whole "mount the existing git source tree" strategy slightly annoying, exactly because of these mismatches between the host and docker uids. Instead, we ended up with something more like: git archive HEAD | docker run ... and the in-container script starts with: tar -x to extract the to-be-tested source, and ends with a dump of the failures to stdout. I don't know if it's worth switching strategies now, but just some food for thought. It may be less interesting with Travis, too, because you're also trying to carry the prove cache across runs, which does require some filesystem interaction. (As an aside, I'm not sure the prove cache is doing much. Running in slow-to-fast order helps if you are trying to run massively in parallel, but we only use -j3 for our Travis builds). -Peff
Re: The original file that was split in 2 other files, is there a way in git to see what went where?
On Mon, Jan 22, 2018 at 7:22 PM, Aleksey Bykovwrote: > Is there an easy way to see: > > 1. what came to b from a? > 2 .what came to c from a? > 3. all extra changes apart from just moving stuff? One way to do this is to use "--color-moved" - it will tell you what in b.txt and c.txt was moved and what wasn't (although it won't tell you from where). This was introduced recently, I think in 2.15.
Re: [PATCH 2/5] travis-ci: use 'set -e' in the 32 bit Linux build job
On Tue, Jan 23, 2018 at 11:26:33AM -0500, Jeff King wrote: > > +HOST_UID=$1 > > +CI_USER=$USER > > +test -z $HOST_UID || (CI_USER="ci" && useradd -u $HOST_UID $CI_USER) > > If this "useradd" step fails, we wouldn't abort the script, because it's > part of a conditional. You'd need a manual "|| exit 1" at the end of > this line. Or to use a real "if" block. > > Reading this line, I'm also slightly confused by setting CI_USER in the > subshell, and then only using it once. Should it be respecting the outer > CI_USER setting? If not, should it just hard-code "ci" on the useradd > command-line? But that has nothing to do with your patch here. OK, nevermind on all this. I just read patch 4. :) -Peff
Re: [PATCH 3/5] travis-ci: don't repeat the path of the cache directory
On Mon, Jan 22, 2018 at 02:32:18PM +0100, SZEDER Gábor wrote: > Some of our 'ci/*' scripts repeat the name or full path of the Travis > CI cache directory, and the following patches will add new places > using that path. > > Use a variable to refer to the path of the cache directory instead, so > it's hard-coded only in a single place. > > Pay extra attention to the 32 bit Linux build: it runs in a Docker > container, so pass the path of the cache directory from the host to > the container in an environment variable. Furthermore, use the > variable in the container only if it's set, to prevent errors when > someone is running or debugging the Docker build locally, because in > that case the variable won't be set as there won't be any Travis CI > cache. Makes sense that we need to pass it down, but... > diff --git a/ci/run-linux32-build.sh b/ci/run-linux32-build.sh > index 248183982b..c9476d6598 100755 > --- a/ci/run-linux32-build.sh > +++ b/ci/run-linux32-build.sh > @@ -25,10 +25,10 @@ CI_USER=$USER > test -z $HOST_UID || (CI_USER="ci" && useradd -u $HOST_UID $CI_USER) > > # Build and test > -linux32 --32bit i386 su -m -l $CI_USER -c ' > +linux32 --32bit i386 su -m -l $CI_USER -c " > set -ex > cd /usr/src/git > - ln -s /tmp/travis-cache/.prove t/.prove > + test -n '$cache_dir' && ln -s '$cache_dir/.prove' t/.prove > make --jobs=2 > make --quiet test > -' > +" This interpolates $cache_dir while generating the snippet. You've stuck it in single-quotes, which prevents most quoting problems, but obviously it's an issue if the variable itself contains a single-quote. Is there a reason not to just export $cache_dir in the environment and access it directly from the snippet? Probably not a _big_ deal, since we control the contents of the variable, but it just seems like a fragile practice to avoid. -Peff
Re: [PATCH 2/5] travis-ci: use 'set -e' in the 32 bit Linux build job
On Mon, Jan 22, 2018 at 02:32:17PM +0100, SZEDER Gábor wrote: > All 'ci/*' scripts use 'set -e' to break the build job if a command > fails, except 'ci/run-linux32-build.sh' which relies on the && chain > to do the same. This inconsistency among the 'ci/*' scripts is asking > for trouble: I forgot about the && chain more than once while working > on this patch series. I think this actually fixes a bug: > # Update packages to the latest available versions > linux32 --32bit i386 sh -c ' > apt update >/dev/null && > apt install -y build-essential libcurl4-openssl-dev libssl-dev \ > libexpat-dev gettext python >/dev/null > -' && > +' If this step failed, then... > # If this script runs inside a docker container, then all commands are > # usually executed as root. Consequently, the host user might not be > # able to access the test output files. > # If a host user id is given, then create a user "ci" with the host user > # id to make everything accessible to the host user. > -HOST_UID=$1 && > -CI_USER=$USER && > -test -z $HOST_UID || (CI_USER="ci" && useradd -u $HOST_UID $CI_USER) && We'd have triggered the right-hand side of this "||", and run the rest of the script. The whole "||" block should have been inside {}. But after your patch, it should be fine with "set -e". Although... > +HOST_UID=$1 > +CI_USER=$USER > +test -z $HOST_UID || (CI_USER="ci" && useradd -u $HOST_UID $CI_USER) If this "useradd" step fails, we wouldn't abort the script, because it's part of a conditional. You'd need a manual "|| exit 1" at the end of this line. Or to use a real "if" block. Reading this line, I'm also slightly confused by setting CI_USER in the subshell, and then only using it once. Should it be respecting the outer CI_USER setting? If not, should it just hard-code "ci" on the useradd command-line? But that has nothing to do with your patch here. -Peff
Re: [PATCH v4 5/6] convert: add 'working-tree-encoding' attribute
On Tue, Jan 23, 2018 at 11:25:58AM +0100, Simon Ruderich wrote: > On Mon, Jan 22, 2018 at 07:54:01PM -0500, Jeff King wrote: > > But anyway, that was a bit of a tangent. Certainly the smaller change is > > just standardizing on sizeof(*foo), which I think most people agree on > > at this point. It might be worth putting in CodingGuidelines. > > Personally I prefer sizeof(*foo) which is a well non-idiom, used > in many projects and IMHO easy to read and understand. Me too. > I've played a little with coccinelle and the following spatch > seems to catch many occurrences of sizeof(struct ..) (the first > hunk seems to expand multiple times causing conflicts in the > generated patch). Cases like a->b = xcalloc() are not matched, I > don't know enough coccinelle for that. If there's interest I > could prepare patches, but it will create quite some code churn. Yeah, I'm not sure what our current policy is there. Traditionally our strategy was not to churn code, but to update old idioms as they were touched. Especially if the change was not urgent, but mostly stylistic (which this one is). But with Coccinelle, it's a lot easier to apply the change tree-wide, and to convert topics in flight as they get merged. The maintainer still gets conflicts with topics-in-flight that touch converted areas, though. So I'd be curious to hear if Junio's opinion has changed at all. -Peff
Re: [PATCH] enable core.fsyncObjectFiles by default
On Tue, Jan 23, 2018 at 12:45:53AM -0500, Theodore Ts'o wrote: > What I was thinking about instead is that in cases where we know we > are likely to be creating a large number of loose objects (whether > they referenced or not), in a world where we will be calling fsync(2) > after every single loose object being created, pack files start > looking *way* more efficient. So in general, if you know you will be > creating N loose objects, where N is probably around 50 or so, you'll > want to create a pack instead. > > One of those cases is "repack -A", and in that case the loose objects > are all going tobe not referenced, so it would be a "cruft pack". But > in many other cases where we might be importing from another DCVS, > which will be another case where doing an fsync(2) after every loose > object creation (and where I have sometimes seen it create them *all* > loose, and not use a pack at all), is going to get extremely slow and > painful. Ah, I see. I think in the general case of git operations this is hard (because most object writes don't realize the larger operation that they're a part of). But I agree that those two are the low-hanging fruit (imports should already be using fast-import, and "cruft packs" are not too hard an idea to implement). I agree that a cruft-pack implementation could just be for "repack -A", and does not have to collect otherwise loose objects. I think part of my confusion was that you and I are coming to the idea from different angles: you care about minimizing fsyncs, and I'm interested in stopping the problem where you have too many loose objects after running auto-gc. So I care more about collecting those loose objects for that case. > > So if we pack all the loose objects into a cruft pack, the mtime of the > > cruft pack becomes the new gauge for "recent". And if we migrate objects > > from old cruft pack to new cruft pack at each gc, then they'll keep > > getting their mtimes refreshed, and we'll never drop them. > > Well, I was assuming that gc would be a special case which doesn't the > mtime of the old cruft pack. (Or more generally, any time an object > is gets copied out of the cruft pack, either to a loose object, or to > another pack, the mtime on the source pack should not be touched.) Right, that's the "you have multiple cruft packs" idea which has been discussed[1] (each one just hangs around until its mtime expires, and may duplicate objects found elsewhere). That does end up with one pack per gc, which just punts the "too many loose objects" to "too many packs". But unless the number of gc runs you do is very high compared to the expiration time, we can probably ignore that. -Peff [1] https://public-inbox.org/git/20170610080626.sjujpmgkli4mu...@sigill.intra.peff.net/
Charity Funds
my name is Mrs. Alice Walton, a business woman an America Citizen and the heiress to the fortune of Walmart stores, born October 7, 1949. I have a mission for you worth $100,000,000.00(Hundred Million United State Dollars) which I intend using for CHARITY
Re: git merge-tree: bug report and some feature requests
On Tue, Jan 23, 2018 at 7:08 AM, Josh Bleecher Snyderwrote: > Looking over your list above, at a minimum, libgit2 might not have a > particularly good way to represent submodule/file or > submodule/directory conflicts, because is-a-submodule is defined > external to a git_index_entry. libgit2 should detect submodule/file and submodule/directory conflicts. While, yes, some metadata about the submodule is located outside the index, you can look at the mode to determine that this _is_ a submodule. You should be able to reliably detect submodule/file conflicts because one will be a regular or executable file (mode 0100644 or 0100755), while the other entry at the same path will be a gitlink (mode 016). Similarly, submodule/directory conflict detection works just like regular file/directory conflict detection. If some path `foo` is a submodule (or a regular file) then some path `foo/bar` existing in the other side of the merge causes a conflict. > Cataloging or special-casing all possible conflict types does seem > unwise because of the sheer number of kinds of conflicts. > > But the alternative appears to be punting entirely, as libgit2 does, > and merely providing something akin to three index entries. Indeed, when I added merge to libgit2, we put the higher-level conflict analysis into application code because there was not much interest in it at the time. I've been meaning to add this to `git_status` in libgit2, but it's not been a high priority. > This which > leaves it unclear what exactly the conflict was, at which point any > user (read: porcelain developer) will end up having to recreate some > merge logic to figure out what went wrong. And if merge-tree starts > doing rename detection, the user might then have to emulate that as > well. That's not a good idea. Trying to figure out what merge did would be painful at best, and likely impossible, since a rename conflict is recorded in the main index without any way to piece it together. eg: 100644 deadbeefdeadbeefdeadbeefdeadbeefdeadbeef 1 bar.c 100644 cafec4fecafec4fecafec4fecafec4fecafec4fe 2 bar.c 100644 c4cc188a892898e13927dc4a02e7f68814b874b2 1 foo.c 100644 71f5af150b25e3d2d67ff46759311401036f 2 foo.c 100644 351cfbdd55d656edd2c5c995aae3caafb9ec11fa 3 rename1.c 100644 e407c7d138fb457674c3b114fcf47748169ab0c5 3 rename2.c This is the main index that results when bar.c has a rename/edit conflict, and foo.c also has a rename/edit conflict. One was renamed to rename1.c and the other to rename2.c. Trying to determine which is which _after the fact_ would be regrettable. Especially since rename detection is not static - you would need to know the thresholds that were configured at the time merge was performed and try to replay the rename detection with that. libgit2 records a `NAME` section in the index that pairs the rename detection decisions that it performed so that you can analyze them and display them after the fact. -ed
Re: [PATCH v4 5/6] convert: add 'working-tree-encoding' attribute
On Mon, Jan 22, 2018 at 07:54:01PM -0500, Jeff King wrote: > But anyway, that was a bit of a tangent. Certainly the smaller change is > just standardizing on sizeof(*foo), which I think most people agree on > at this point. It might be worth putting in CodingGuidelines. Personally I prefer sizeof(*foo) which is a well non-idiom, used in many projects and IMHO easy to read and understand. I've played a little with coccinelle and the following spatch seems to catch many occurrences of sizeof(struct ..) (the first hunk seems to expand multiple times causing conflicts in the generated patch). Cases like a->b = xcalloc() are not matched, I don't know enough coccinelle for that. If there's interest I could prepare patches, but it will create quite some code churn. Regards Simon @@ type T; identifier x; @@ - T *x = xmalloc(sizeof(T)); + T *x = xmalloc(sizeof(*x)); @@ type T; T *x; @@ - x = xmalloc(sizeof(T)); + x = xmalloc(sizeof(*x)); @@ type T; identifier x; expression n; @@ - T *x = xcalloc(n, sizeof(T)); + T *x = xcalloc(n, sizeof(*x)); @@ type T; T *x; expression n; @@ - x = xcalloc(n, sizeof(T)); + x = xcalloc(n, sizeof(*x)); @@ type T; T x; @@ - memset(, 0, sizeof(T)); + memset(, 0, sizeof(x)); @@ type T; T *x; @@ - memset(x, 0, sizeof(T)); + memset(x, 0, sizeof(*x)); -- + privacy is necessary + using gnupg http://gnupg.org + public key id: 0x92FEFDB7E44C32F9
[PATCH v2 2/2] sequencer: run 'prepare-commit-msg' hook
From: Phillip WoodCommit 356ee4659b ("sequencer: try to commit without forking 'git commit'", 2017-11-24) forgot to run the 'prepare-commit-msg' hook when creating the commit. Fix this by writing the commit message to a different file and running the hook. Using a different file means that if the commit is cancelled the original message file is unchanged. Also move the checks for an empty commit so the order matches 'git commit'. Reported-by: Dmitry Torokhov Signed-off-by: Phillip Wood Reviewed-by: Ramsay Jones --- Notes: Changes since v1: - marked run_prepare_commit_msg_hook() as static (Thanks to Ramsey Jones) builtin/commit.c | 2 -- sequencer.c| 69 +++--- sequencer.h| 1 + t/t7505-prepare-commit-msg-hook.sh | 8 ++--- 4 files changed, 61 insertions(+), 19 deletions(-) diff --git a/builtin/commit.c b/builtin/commit.c index 4a64428ca8ed8c3066aec7cfd8ad7b71217af7dd..5dd766af2842dddb80d30cd73b8be8ccb4956eac 100644 --- a/builtin/commit.c +++ b/builtin/commit.c @@ -66,8 +66,6 @@ N_("If you wish to skip this commit, use:\n" "Then \"git cherry-pick --continue\" will resume cherry-picking\n" "the remaining commits.\n"); -static GIT_PATH_FUNC(git_path_commit_editmsg, "COMMIT_EDITMSG") - static const char *use_message_buffer; static struct lock_file index_lock; /* real index */ static struct lock_file false_lock; /* used only for partial commits */ diff --git a/sequencer.c b/sequencer.c index 63a8ec9a61e7a7bf603ffa494621af79b60e0b76..5bfdc4044233d5f809f9f1fbc55ebe3da477e3f0 100644 --- a/sequencer.c +++ b/sequencer.c @@ -29,6 +29,8 @@ const char sign_off_header[] = "Signed-off-by: "; static const char cherry_picked_prefix[] = "(cherry picked from commit "; +GIT_PATH_FUNC(git_path_commit_editmsg, "COMMIT_EDITMSG") + GIT_PATH_FUNC(git_path_seq_dir, "sequencer") static GIT_PATH_FUNC(git_path_todo_file, "sequencer/todo") @@ -891,6 +893,31 @@ void commit_post_rewrite(const struct commit *old_head, run_rewrite_hook(_head->object.oid, new_head); } +static int run_prepare_commit_msg_hook(struct strbuf *msg, const char *commit) +{ + struct argv_array hook_env = ARGV_ARRAY_INIT; + int ret; + const char *name; + + name = git_path_commit_editmsg(); + if (write_message(msg->buf, msg->len, name, 0)) + return -1; + + argv_array_pushf(_env, "GIT_INDEX_FILE=%s", get_index_file()); + argv_array_push(_env, "GIT_EDITOR=:"); + if (commit) + ret = run_hook_le(hook_env.argv, "prepare-commit-msg", name, + "commit", commit, NULL); + else + ret = run_hook_le(hook_env.argv, "prepare-commit-msg", name, + "message", NULL); + if (ret) + ret = error(_("'prepare-commit-msg' hook failed")); + argv_array_clear(_env); + + return ret; +} + static const char implicit_ident_advice_noconfig[] = N_("Your name and email address were configured automatically based\n" "on your username and hostname. Please check that they are accurate.\n" @@ -1051,8 +1078,9 @@ static int try_to_commit(struct strbuf *msg, const char *author, struct commit_list *parents = NULL; struct commit_extra_header *extra = NULL; struct strbuf err = STRBUF_INIT; - struct strbuf amend_msg = STRBUF_INIT; + struct strbuf commit_msg = STRBUF_INIT; char *amend_author = NULL; + const char *hook_commit = NULL; enum commit_msg_cleanup_mode cleanup; int res = 0; @@ -1069,8 +1097,9 @@ static int try_to_commit(struct strbuf *msg, const char *author, const char *orig_message = NULL; find_commit_subject(message, _message); - msg = _msg; + msg = _msg; strbuf_addstr(msg, orig_message); + hook_commit = "HEAD"; } author = amend_author = get_author(message); unuse_commit_buffer(current_head, message); @@ -1084,16 +1113,6 @@ static int try_to_commit(struct strbuf *msg, const char *author, commit_list_insert(current_head, ); } - cleanup = (flags & CLEANUP_MSG) ? COMMIT_MSG_CLEANUP_ALL : - opts->default_msg_cleanup; - - if (cleanup != COMMIT_MSG_CLEANUP_NONE) - strbuf_stripspace(msg, cleanup == COMMIT_MSG_CLEANUP_ALL); - if (!opts->allow_empty_message && message_is_empty(msg, cleanup)) { - res = 1; /* run 'git commit' to display error message */ - goto out; - } - if (write_cache_as_tree(tree.hash, 0, NULL)) { res = error(_("git write-tree failed
[PATCH v2 0/2] sequencer: run 'prepare-commit-msg' hook
From: Phillip WoodI've updated the patches in response to comments, there are just a couple of small changes. Thanks to Ramsay and Eric for their reviews. Best Wishes Phillip Original cover letter: These two patches add some tests and fix the sequencer to run the 'prepare-commit-msg' hook when committing without forking 'git commit' Phillip Wood (2): t7505: Add tests for cherry-pick and rebase -i/-p sequencer: run 'prepare-commit-msg' hook builtin/commit.c | 2 - sequencer.c| 69 sequencer.h| 1 + t/t7505-prepare-commit-msg-hook.sh | 127 +++-- t/t7505/expected-rebase-i | 17 + t/t7505/expected-rebase-p | 18 ++ 6 files changed, 215 insertions(+), 19 deletions(-) create mode 100644 t/t7505/expected-rebase-i create mode 100644 t/t7505/expected-rebase-p -- 2.15.1
[PATCH v2 1/2] t7505: Add tests for cherry-pick and rebase -i/-p
From: Phillip WoodCheck that cherry-pick and rebase call the 'prepare-commit-msg' hook correctly. The expected values for the hook arguments are taken to match the current master branch. I think there is scope for improving the arguments passed so they make a bit more sense - for instance cherry-pick currently passes different arguments depending on whether the commit message is being edited. Also the arguments for rebase could be improved. Commit 7c4188360ac ("rebase -i: proper prepare-commit-msg hook argument when squashing", 2008-10-3) apparently changed things so that when squashing rebase would pass 'squash' as the argument to the hook but that has been lost. I think that it would make more sense to pass 'message' for revert and cherry-pick -x/-s (i.e. cases where there is a new message or the current message in modified by the command), 'squash' when squashing with a new message and 'commit HEAD/CHERRY_PICK_HEAD' otherwise (picking and squashing without a new message). Signed-off-by: Phillip Wood Reviewed-by: Eric Sunshine --- Notes: Changes since v1 - use test_seq for portability (Thanks to Eric Sunshine) t/t7505-prepare-commit-msg-hook.sh | 127 +++-- t/t7505/expected-rebase-i | 17 + t/t7505/expected-rebase-p | 18 ++ 3 files changed, 158 insertions(+), 4 deletions(-) create mode 100644 t/t7505/expected-rebase-i create mode 100644 t/t7505/expected-rebase-p diff --git a/t/t7505-prepare-commit-msg-hook.sh b/t/t7505-prepare-commit-msg-hook.sh index b13f72975ecce17887c4c8275c6935d78d4b09a0..a3dd3545030c2a29a1ca4502f26b412f92f0375a 100755 --- a/t/t7505-prepare-commit-msg-hook.sh +++ b/t/t7505-prepare-commit-msg-hook.sh @@ -4,6 +4,38 @@ test_description='prepare-commit-msg hook' . ./test-lib.sh +test_expect_success 'set up commits for rebasing' ' + test_commit root && + test_commit a a a && + test_commit b b b && + git checkout -b rebase-me root && + test_commit rebase-a a aa && + test_commit rebase-b b bb && + for i in $(test_seq 1 13) + do + test_commit rebase-$i c $i + done && + git checkout master && + + cat >rebase-todo <<-EOF + pick $(git rev-parse rebase-a) + pick $(git rev-parse rebase-b) + fixup $(git rev-parse rebase-1) + fixup $(git rev-parse rebase-2) + pick $(git rev-parse rebase-3) + fixup $(git rev-parse rebase-4) + squash $(git rev-parse rebase-5) + reword $(git rev-parse rebase-6) + squash $(git rev-parse rebase-7) + fixup $(git rev-parse rebase-8) + fixup $(git rev-parse rebase-9) + edit $(git rev-parse rebase-10) + squash $(git rev-parse rebase-11) + squash $(git rev-parse rebase-12) + edit $(git rev-parse rebase-13) + EOF +' + test_expect_success 'with no hook' ' echo "foo" > file && @@ -31,17 +63,40 @@ mkdir -p "$HOOKDIR" echo "#!$SHELL_PATH" > "$HOOK" cat >> "$HOOK" <<'EOF' +GIT_DIR=$(git rev-parse --git-dir) +if test -d "$GIT_DIR/rebase-merge" +then + rebasing=1 +else + rebasing=0 +fi + +get_last_cmd () { + tail -n1 "$GIT_DIR/rebase-merge/done" | { +read cmd id _ +git log --pretty="[$cmd %s]" -n1 $id + } +} + if test "$2" = commit; then - source=$(git rev-parse "$3") + if test $rebasing = 1 + then +source="$3" + else +source=$(git rev-parse "$3") + fi else source=${2-default} fi -if test "$GIT_EDITOR" = :; then - sed -e "1s/.*/$source (no editor)/" "$1" > msg.tmp +test "$GIT_EDITOR" = : && source="$source (no editor)" + +if test $rebasing = 1 +then + echo "$source $(get_last_cmd)" >"$1" else sed -e "1s/.*/$source/" "$1" > msg.tmp + mv msg.tmp "$1" fi -mv msg.tmp "$1" exit 0 EOF chmod +x "$HOOK" @@ -156,6 +211,63 @@ test_expect_success 'with hook and editor (merge)' ' test "$(git log -1 --pretty=format:%s)" = "merge" ' +test_rebase () { + expect=$1 && + mode=$2 && + test_expect_$expect C_LOCALE_OUTPUT "with hook (rebase $mode)" ' + test_when_finished "\ + git rebase --abort + git checkout -f master + git branch -D tmp" && + git checkout -b tmp rebase-me && + GIT_SEQUENCE_EDITOR="cp rebase-todo" && + GIT_EDITOR="\"$FAKE_EDITOR\"" && + ( + export GIT_SEQUENCE_EDITOR GIT_EDITOR && + test_must_fail git rebase $mode b && + echo x>a && + git add a && + test_must_fail git rebase --continue && + echo x>b && + git add b && + git commit && + git rebase --continue && + echo y>a && + git add a &&
[PATCH v2] SQUASH convert: add tracing for 'working-tree-encoding' attribute
From: Lars SchneiderHi Junio, I overlooked a typo pointed out in Simon's review. Here is a new patch for squashing. Sorry for the trouble! @Eric: Thanks for spotting this! Cheers, Lars convert.c| 8 ++-- t/t0028-working-tree-encoding.sh | 2 ++ 2 files changed, 8 insertions(+), 2 deletions(-) diff --git a/convert.c b/convert.c index 13fad490ce..dce2f6e201 100644 --- a/convert.c +++ b/convert.c @@ -1165,8 +1165,12 @@ static struct encoding *git_path_check_encoding(struct attr_check_item *check) if (!strcasecmp(value, default_encoding)) return NULL; - enc = xcalloc(1, sizeof(struct convert_driver)); - enc->name = xstrdup_toupper(value); /* aways use upper case names! */ + enc = xcalloc(1, sizeof(*enc)); + /* +* Ensure encoding names are always upper case (e.g. UTF-8) to +* simplify subsequent string comparisons. +*/ + enc->name = xstrdup_toupper(value); *encoding_tail = enc; encoding_tail = &(enc->next); diff --git a/t/t0028-working-tree-encoding.sh b/t/t0028-working-tree-encoding.sh index 0f36d4990a..a6da61280d 100755 --- a/t/t0028-working-tree-encoding.sh +++ b/t/t0028-working-tree-encoding.sh @@ -22,6 +22,8 @@ test_expect_success 'setup test repo' ' test_expect_success 'ensure UTF-8 is stored in Git' ' git cat-file -p :test.utf16 >test.utf16.git && test_cmp_bin test.utf8.raw test.utf16.git && + + # cleanup rm test.utf8.raw test.utf16.git ' base-commit: 21f4dac5aba07a6109285c57a0478bf502e09009 -- 2.16.0
Re: [PATCH/RFC 0/2] Automate updating git-completion.bash a bit
On Mon, Jan 22, 2018 at 07:03:24PM +0100, SZEDER Gábor wrote: > On Wed, Jan 17, 2018 at 10:34 AM, Duy Nguyenwrote: > > Actually I forgot another option. What if we automate updating the > > script at "compile" time instead of calling git at run time? E.g. with > > something like below, a contributor could just run > > > > make update-completion > > > > then add git-completion.bash changes to the same patch that introduces > > new options. If they forget > > They inevitably will :) OK. We go with the first option then (with caching to reduce overhead on Windows). I'm not going to bother you with actual code changes. The diff of completable options looks like below. It would be great if people could check if some options should NOT be completable for some reason. A couple points - Ignore options that are removed in the diff. They are removed for a reason which I will explain when real patches come. - There are regressions where --foo= becomes --foo, I hope this is ok for now. We can tweak this later. - In some commands you can complete both --foo and --foo= (now it's just one form, --foo). I would argue that it's pointless. It's no big deal to type '=' (or a space) after we have completed --foo. - I feel --force is not treated the same way everywhere. But I guess that's ok since "forcing" in some command context may be less severe than others. -- 8< -- diff --git a/git-add.txt b/git-add.txt index 1c249a3..2693cc1 100644 --- a/git-add.txt +++ b/git-add.txt @@ -1,10 +1,15 @@ +--all --chmod= --dry-run --edit --force --ignore-errors +--ignore-missing +--ignore-removal --intent-to-add --interactive --patch --refresh +--renormalize --update +--verbose diff --git a/git-am.txt b/git-am.txt index b0082bf..552dc96 100644 --- a/git-am.txt +++ b/git-am.txt @@ -1,12 +1,24 @@ --3way --committer-date-is-author-date +--directory +--exclude +--gpg-sign --ignore-date --ignore-space-change --ignore-whitespace +--include --interactive --keep +--keep-cr +--keep-non-patch +--message-id +--no-keep-cr --no-utf8 +--patch-format +--quiet +--reject +--resolvemsg= --scissors --signoff --utf8 ---whitespace= +--whitespace diff --git a/git-apply.txt b/git-apply.txt index 6bf4c2f..71d53d2 100644 --- a/git-apply.txt +++ b/git-apply.txt @@ -1,13 +1,17 @@ +--3way +--allow-overlap --apply +--build-fake-ancestor= --cached --check ---directory= ---exclude= +--directory +--exclude --ignore-space-change --ignore-whitespace --inaccurate-eof +--include --index ---index-info +--no-add --no-add --numstat --recount @@ -17,4 +21,4 @@ --summary --unidiff-zero --verbose ---whitespace= +--whitespace diff --git a/git-branch.txt b/git-branch.txt index 69594e3..9d308aa 100644 --- a/git-branch.txt +++ b/git-branch.txt @@ -1,10 +1,14 @@ ---abbrev= +--abbrev +--all --color --column --contains --copy +--create-reflog --delete --edit-description +--format= +--ignore-case --list --merged --move @@ -15,9 +19,10 @@ --no-merged --no-track --points-at +--quiet --remotes --set-upstream-to= ---sort= +--sort --track --unset-upstream --verbose diff --git a/git-checkout.txt b/git-checkout.txt index 493a1fe..75f19d2 100644 --- a/git-checkout.txt +++ b/git-checkout.txt @@ -1,12 +1,14 @@ --conflict= --detach +--ignore-other-worktrees --ignore-skip-worktree-bits --merge --no-recurse-submodules --no-track ---orphan +--orphan= --ours --patch +--progress --quiet --recurse-submodules --theirs diff --git a/git-cherry-pick.txt b/git-cherry-pick.txt index 39ba895..f8cdbce 100644 --- a/git-cherry-pick.txt +++ b/git-cherry-pick.txt @@ -1,5 +1,14 @@ +--abort +--allow-empty +--allow-empty-message +--continue --edit +--ff +--gpg-sign +--keep-redundant-commits --mainline --no-commit +--quit --signoff --strategy= +--strategy-option diff --git a/git-clean.txt b/git-clean.txt index 40407f7..10c6155 100644 --- a/git-clean.txt +++ b/git-clean.txt @@ -1,2 +1,4 @@ --dry-run +--exclude +--interactive --quiet diff --git a/git-clone.txt b/git-clone.txt index f6e892b..1b6a4da 100644 --- a/git-clone.txt +++ b/git-clone.txt @@ -1,18 +1,29 @@ --bare ---branch ---depth +--branch= +--config +--depth= +--dissociate +--ipv4 +--ipv6 +--jobs= --local --mirror --no-checkout --no-hardlinks --no-single-branch --no-tags ---origin +--origin= +--progress --quiet --recurse-submodules --reference +--reference-if-able +--separate-git-dir= +--shallow-exclude +--shallow-since= --shallow-submodules --shared --single-branch --template= ---upload-pack +--upload-pack= +--verbose diff --git a/git-commit.txt b/git-commit.txt index 2f98a59..337a57e 100644 --- a/git-commit.txt +++ b/git-commit.txt @@ -1,20 +1,26 @@ --all ---allow-empty --amend --author= +--branch --cleanup= ---date +--date= --dry-run --edit --file= --fixup= +--gpg-sign --include --interactive ---message= +--long +--message --no-edit +--no-post-rewrite --no-verify +--no-verify +--null --only --patch +--porcelain --quiet
Re: [PATCH v4 5/6] convert: add 'working-tree-encoding' attribute
On Mon, Jan 22, 2018 at 01:35:25PM +0100, Lars Schneider wrote: >>> + enc->name = xstrdup_toupper(value); /* aways use upper case names! */ >> >> "aways" -> "always" and I think the comment should say why >> uppercase is important. > > Would that be better? > > /* Aways use upper case names to simplify subsequent string comparison. > */ > enc->name = xstrdup_toupper(value); > > AFAIK uppercase and lowercase names are both valid. I just wanted to > ensure that we use one consistent casing. That reads better in error messages > and I don't need to check for the letter case in has_prohibited_utf_bom() > and friends in utf8.c Sounds good (minus the "Aways" typo Eric already noticed). >> Micro-nit: For consistency with the previous test, remove the >> empty line and comment (or just keep the files generated from the >> "setup test repo" phase and don't explicitly delete them)? > > I would rather add a new line and a comment to the previous test > to be consistent. > > I know we could leave the files but these lingering files could > always surprise writers of future tests (at least they surprised > me in other tests). Sure, that sounds good. Just noticed the inconsistency and wanted to mention it. Regards Simon -- + privacy is necessary + using gnupg http://gnupg.org + public key id: 0x92FEFDB7E44C32F9