Re: no mountable file systems
On Mon, Nov 13, 2017 at 10:35 PM, Louis Gruandwrote: > Dear team, when i download Git on Mac it says “no mountable file systems” and > doesnt open after. how can i solve this? When googling "Mac no mountable file systems", it looks like this error is not specific to Git. Could you check if you have the same problem when downloading and installing other software? If that is the case, you might want to get help from Mac specific forums, mailing lists or support channels rather than from this list.
Re: [PATCH V2] config: add --expiry-date
h...@unimetic.com writes: > From: Haaris> > Description: > This patch adds a new option to the config command. > > Uses flag --expiry-date as a data-type to covert date-strings to > timestamps when reading from config files (GET). > This flag is ignored on write (SET) because the date-string is stored in > config without performing any normalization. > > Creates a few test cases and documentation since its a new feature. > > Motivation: > A parse_expiry_date() function already existed for api calls, > this patch simply allows the function to be used from the command line. > > Signed-off-by: Haaris > --- Please drop all these section headers; they are irritating. Learn from "git log --no-merges" how the log messages in this project is written and imitate them. Documentation/SubmittingPatches would be helpful. Add --expiry-date as a new type 'git config --get' takes, similar to existing --int, --bool, etc. types, so that scripts can learn values of configuration variables like gc.reflogexpire (e.g. "2.weeks") in a more useful way (e.g. the timesamp as of two weeks ago, expressed in number of seconds since epoch). As a helper function necessary to do this already exists in the implementation of builtin/reflog.c, the implementation is just the matter of moving it to config.c and using it from bultin/config.c, but shuffle the order of the parameter so that the pointer to the output variable comes first. This is to match the convention used by git_config_pathname() and other helper functions. or something like that? > + } else if (types == TYPE_EXPIRY_DATE) { > + timestamp_t t; > + if(git_config_expiry_date(, key_, value_) < 0) Style. if (git_config_expiry_date(, key_, value_) < 0) > + return -1; > + strbuf_addf(buf, "%"PRItime, t); > ... Thanks.
Re: [PATCH V2] config: add --expiry-date
On Tue, Nov 14, 2017 at 3:04 AM,wrote: > From: Haaris > > Description: > This patch adds a new option to the config command. > > Uses flag --expiry-date as a data-type to covert date-strings to > timestamps when reading from config files (GET). > This flag is ignored on write (SET) because the date-string is stored in > config without performing any normalization. > > Creates a few test cases and documentation since its a new feature. > > Motivation: > A parse_expiry_date() function already existed for api calls, > this patch simply allows the function to be used from the command line. > > Signed-off-by: Haaris Documentation/SubmittingPatches contains the following: "Also notice that a real name is used in the Signed-off-by: line. Please don't hide your real name." And there is the following example before that: Signed-off-by: Random J Developer So it looks like "a real name" actually means "a real firstname and a real surname". It would be nice if your "Signed-off-by:" could match this format. Also if you have a "From:" line at the beginning of the patch, please make sure that the name there is tha same as on the "Signed-off-by:". Thanks for working on this, Christian.
Re: What's cooking in git.git (Nov 2017, #04; Tue, 14)
Kaartic Sivaraamwrites: >> * jc/branch-name-sanity (2017-10-14) 3 commits >> - branch: forbid refs/heads/HEAD >> - branch: split validate_new_branchname() into two >> - branch: streamline "attr_only" handling in validate_new_branchname() >> >> "git branch" and "git checkout -b" are now forbidden from creating >> a branch whose name is "HEAD". >> >> Reported to cause problems when renaming HEAD during a rebase. >> cf. <49563f7c-354e-334e-03a6-c3a40884b...@gmail.com> > > > Just wanted to note this explicitly. As I'm not aware how the problem > with above series is going to be resolved, I've decided to stall the > v4 of my series that tries to improve error messages shown when > renaming the branch[1] until this problem gets resolved. I'm doing > this as this series and my series touch the same code > paths. Furthermore, I based my v3 off of 'next' when this series was > in there. > > I'm not sure if the resolution to the problem might introduce > conflicts with my series. Hence the stall. It is not like the original author of a series _owns_ the code; it is open source after all. So if you are inclined to, you are welcome to fix it up or rewrite it, if somebody else's series that is not actively being worked on needs updating before you can continue your work.
Re: [RFC 3/3] log: add an option to generate cover letter from a branch tip
Nicolas Morey-Chaisemartinwrites: > - const char *body = "*** SUBJECT HERE ***\n\n*** BLURB HERE ***\n"; > - const char *msg; > + const char *body = "*** SUBJECT HERE ***\n\n*** BLURB HERE ***\n\n"; H. > @@ -1021,17 +1021,21 @@ static void make_cover_letter(struct rev_info *rev, > int use_stdout, > if (!branch_name) > branch_name = find_branch_name(rev); > > - msg = body; > pp.fmt = CMIT_FMT_EMAIL; > pp.date_mode.type = DATE_RFC2822; > pp.rev = rev; > pp.print_email_subject = 1; > - pp_user_info(, NULL, , committer, encoding); > - pp_title_line(, , , encoding, need_8bit_cte); > - pp_remainder(, , , 0); > - add_branch_description(, branch_name); > - fprintf(rev->diffopt.file, "%s\n", sb.buf); > > + if (!cover_at_tip_commit) { > + pp_user_info(, NULL, , committer, encoding); > + pp_title_line(, , , encoding, need_8bit_cte); > + pp_remainder(, , , 0); > + } else { > + pretty_print_commit(, cover_at_tip_commit, ); > + } > + add_branch_description(, branch_name); > + fprintf(rev->diffopt.file, "%s", sb.buf); > + fprintf(rev->diffopt.file, "---\n", sb.buf); > strbuf_release(); I would have expected that this feature would not change anything other than replacing the constant string *body we unconditionally print with the log message of the empty commit at the tip, so from that expectation, I was hoping that a patch looked nothing more than this: builtin/log.c | 6 +- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/builtin/log.c b/builtin/log.c index 6c1fa896ad..0af19d5b36 100644 --- a/builtin/log.c +++ b/builtin/log.c @@ -986,6 +986,7 @@ static void make_cover_letter(struct rev_info *rev, int use_stdout, struct commit *origin, int nr, struct commit **list, const char *branch_name, + struct commit *cover, int quiet) { const char *committer; @@ -1021,7 +1022,10 @@ static void make_cover_letter(struct rev_info *rev, int use_stdout, if (!branch_name) branch_name = find_branch_name(rev); - msg = body; + if (cover) + msg = get_cover_from_commit(cover); + else + msg = body; pp.fmt = CMIT_FMT_EMAIL; pp.date_mode.type = DATE_RFC2822; pp.rev = rev; plus a newly written function get_cover_from_commit(). Why does this patch need to change a lot more than that, I have to wonder. This is totally unrelated, but I wonder if it makes sense to do something similar for branch.description, too. If the user has a meaningful description prepared with "git branch --edit-desc", it is somewhat insulting to the user to still add "*** BLURB HERE ***".
Re: [RFC 2/3] am: semi working --cover-at-tip
Nicolas Morey-Chaisemartinwrites: > if (!git_config_get_bool("commit.gpgsign", )) > state->sign_commit = gpgsign ? "" : NULL; > + > } Please give at least a cursory proof-reading before sending things out. > @@ -1106,14 +1131,6 @@ static void am_next(struct am_state *state) > > oidclr(>orig_commit); > unlink(am_path(state, "original-commit")); > - > - if (!get_oid("HEAD", )) > - write_state_text(state, "abort-safety", oid_to_hex()); > - else > - write_state_text(state, "abort-safety", ""); > - > - state->cur++; > - write_state_count(state, "next", state->cur); Moving these lines to a later part of the source file is fine, but can you do so as a separate preparatory patch that does not change anything else? That would unclutter the main patch that adds the feature, allowing better reviews from reviewers. The hunk below... > +/** > + * Increments the patch pointer, and cleans am_state for the application of > the > + * next patch. > + */ > +static void am_next(struct am_state *state) > +{ > + struct object_id head; > + > + /* Flush the cover letter if needed */ > + if (state->cover_at_tip == 1 && > + state->series_len > 0 && > + state->series_id == state->series_len && > + state->cover_id > 0) > + do_apply_cover(state); > + > + am_clean(state); > + > + if (!get_oid("HEAD", )) > + write_state_text(state, "abort-safety", oid_to_hex()); > + else > + write_state_text(state, "abort-safety", ""); > + > + state->cur++; > + write_state_count(state, "next", state->cur); > +} ... if you followed that "separate preparatory step" approach, would show clearly that you added the logic to call do_apply_cover() when we transition after applying the Nth patch of a series with N patches, as all the existing lines will show only as unchanged context lines. By the way, don't we want to sanity check state->last (which we learn by running "git mailsplit" that splits the incoming mbox into pieces and counts the number of messages) against state->series_len? Sometimes people send [PATCH 0-6/6], a 6-patch series with a cover letter, and then follow-up with [PATCH 7/6]. For somebody like me, it would be more convenient if the above code (more-or-less) ignored series_len and called do_apply_cover() after applying the last patch (which would be [PATCH 7/6]) based on what state->last says. Thanks.
Re: Recovering from gc errors
On Mon, Nov 13, 2017 at 04:13:19PM -0500, Marc Branchaud wrote: > Various incantations of "git show ... 9c355a7726e31" only fail with the same > error, so I can't determine much about the problematic commit. Luckily I'm > not particularly concerned with losing objects, as I push any important > progress to named refs in backup repos. Doing "git show" will require looking at the parent commit to produce the diff. Probably "git show -s" would work. But in general for poking at corruption, something bare-bones like "git cat-file commit 9c355a77" is going to be your best bet. > But I would like to clean this up in my local repo so that gc stops failing. > I tried simply removing this and other loose commits that trip up gc (i.e. > the objects/9c/355a7726e31b3033b8e714cf7edb4f0a41d8d4 file -- there are 49 > such files, all of which are several months old), but now gc complains of a > bad tree object: You can't generally fix corruption issues by deleting objects[1]. The "source" that makes Git want to have these objects is the refs and reflogs. So your best bet is to find which of those point to the problematic objects and delete them. I'd start by seeing if the breakage is reachable from any refs: git rev-list --objects --all >/dev/null If that command succeeds, then all your refs are intact and the problem is in the reflogs. You can try to figure out which, but I'd probably just blow them all away: rm -rf .git/logs If the rev-list fails, then one or more branch is corrupted. Unfortunately the usual efficient tools for asking "which branch contains this object" are likely to be broken by the corruption. But you can brute-force it, like: git for-each-ref --format='%(refname)' | while read ref; do git rev-list --objects "$ref" >/dev/null 2>&1 || echo "$ref is broken" done Hopefully that turns up only branches with little value, and you can delete them: git update-ref -d $broken_ref -Peff [1] A note on my "you can't fix corruption by deleting objects". Since abcb86553d (pack-objects: match prune logic for discarding objects, 2014-10-15) , git-gc also traverses the history graph of unreachable but "recent" objects. This is to keep whole chunks of the history graph intact during the gc grace period (which is 2 weeks by default). So object themselves _can_ be a source of traversal for git-gc. We do that traversal with the ignore_missing_links flag, so breakages in the unreachable objects _shouldn't_ cause what you're seeing. IIRC we did turn up a bug or two with ignore_missing_links. The only one I could find was a3ba6bf10a (revision.c: ignore broken tags with ignore_missing_links, 2017-05-20), which I think wouldn't generate the output you're seeing.
Re: [RFC 1/3] mailinfo: extract patch series id
Nicolas Morey-Chaisemartinwrites: > Extract the patch ID and series length from the [PATCH N/M] > prefix in the mail header > > Signed-off-by: Nicolas Morey-Chaisemartin > --- > mailinfo.c | 35 +++ > mailinfo.h | 2 ++ > 2 files changed, 37 insertions(+) As JTan already mentioned, relying on a substring "PATCH" may not be very reliable, and trying to locate "%d/%d]" feels like a better approach. cleanup_subject() is called only when keep_subject is false, so this code will not trigger in that case at all. Is this intended? I would have expected that a new helper function would be written, without changing existing helpers like cleanup_subject(), and that new helper gets called by handle_info() after output_header_lines() helper is called for the "Subject". Whenever mailinfo learns to glean a new useful piece of information, it should be made available to scripts that run "git mailinfo", too. Perhaps show something like PatchNumber: 1 TotalPatches: 3 at the end of handle_info() to mi->output? I do not think existing tools mind too much, even if we added a for-debug output e.g. RawSubject: [RFC 1/3] mailinfo: extract patch series id to the output.
Re: What's cooking in git.git (Nov 2017, #04; Tue, 14)
* jc/branch-name-sanity (2017-10-14) 3 commits - branch: forbid refs/heads/HEAD - branch: split validate_new_branchname() into two - branch: streamline "attr_only" handling in validate_new_branchname() "git branch" and "git checkout -b" are now forbidden from creating a branch whose name is "HEAD". Reported to cause problems when renaming HEAD during a rebase. cf. <49563f7c-354e-334e-03a6-c3a40884b...@gmail.com> Just wanted to note this explicitly. As I'm not aware how the problem with above series is going to be resolved, I've decided to stall the v4 of my series that tries to improve error messages shown when renaming the branch[1] until this problem gets resolved. I'm doing this as this series and my series touch the same code paths. Furthermore, I based my v3 off of 'next' when this series was in there. I'm not sure if the resolution to the problem might introduce conflicts with my series. Hence the stall. -- [1]: https://public-inbox.org/git/20171102065407.25404-1-kaartic.sivar...@gmail.com/ --- Kaartic
Re: [PATCH 21/30] merge-recursive: Add get_directory_renames()
Elijah Newrenwrites: > + entry = dir_rename_find_entry(dir_renames, old_dir); > + if (!entry) { > + entry = xcalloc(1, sizeof(struct dir_rename_entry)); > + hashmap_entry_init(entry, strhash(old_dir)); Please make these two lines into its own dir_rename_entry_init() helper. Because the structure is defined as +struct dir_rename_entry { + struct hashmap_entry ent; /* must be the first member! */ + char *dir; + unsigned non_unique_new_dir:1; + char *new_dir; + struct string_list possible_new_dirs; +}; + in the previous patch, we'd want to see its string_list member to be initialised explicitly (we do not want to depend on "filling with NUL happens to make it a NODUP kind of string_list, which suits our purpose"). The definition of _init() function may belong to the previous step.
Re: [RFC PATCH v3 4/4] builtin/branch: give more useful error messages when renaming
On Monday 13 November 2017 05:00 PM, Kevin Daudt wrote: On Mon, Nov 13, 2017 at 08:01:12AM +0530, Kaartic Sivaraam wrote: That was a little attribution I wanted make to the strbuf API as this was the first time I leveraged it to this extent and I was surprised by the way it made string manipulation easier in C. Just documented my excitation. In case it seems to be noise (?) which should removed, let me know. I guess that would fit better below the the --- That's a nice point. Thanks. (Why didn't I think of it before) --- Kaartic
Re: [PATCH 19/30] merge-recursive: Split out code for determining diff_filepairs
Elijah Newrenwrites: > Create a new function, get_diffpairs() to compute the diff_filepairs > between two trees. While these are currently only used in > get_renames(), I want them to be available to some new functions. No > actual logic changes yet. OK. This refactors an easy-to-use (in the context of merge-recursive code) wrapper to diff-tree out of the existing code, which makes sense.
Re: [PATCH] t/lib-gpg: fix gpgconf stderr redirect to /dev/null
Junio C Hamano wrote: The message goes to the standard output stream since it was introduced in 809f38c8 ("git notes merge: Manual conflict resolution, part 1/2", 2010-11-09) and 6abb3655 ("git notes merge: Manual conflict resolution, part 2/2", 2010-11-09). I do think it makes more sense to send it to the standard error stream, but just in case if the original author thinks of a reason why it shouldn't, let's summon Johan and ask his input. Sounds like a good plan. If the message does move to stderr, there are also a few tests in 3310 that need adjusted. They presume an error message from `git notes merge`, but they only redirect stdout to the output file. While I was bored, I prepared a commit with these changes and confirmed the test suite passes, in case we get an ACK from Johan. -- Todd ~~ It is impossible to enjoy idling thoroughly unless one has plenty of work to do. -- Jerome K. Jerome
Re: [PATCH 16/30] merge-recursive: Introduce new functions to handle rename logic
Junio C Hamanowrites: > Elijah Newren writes: > >> +struct rename_info { >> +struct string_list *head_renames; >> +struct string_list *merge_renames; >> +}; > > This type is added in order to allow the caller and the helper to > communicate the findings in a single logical structure, instead of > having to pass them as separate parameters, etc. If we anticipate > that the information that needs to be passed will grow richer in > later steps (or a follow-up series), such encapsulation makes a lot Hmph, I actually am quite confused with the existing code. The caller (originally in merge_trees(), now in handle_renames()) calls get_renames() twice and have the list of renamed paths in these two string lists. get_renames() mostly works with the elements in the "entries" list and adds the "struct rename" to the string list that is to be returned. And the caller uses these two string lists get_renames() returns when calling process_renames(), but once process_renames() is done with them, these two string lists are never looked at by anybody. So do we really need to pass this structure around in the first place? I am wondering if we can do the cleanup_rename() on both of these lists after handle_renames() calls process_renames() before returning, which will eliminate the need for this structure and a separate cleanup_renames() helper that clears the structure and the two string lists in it.
Re: [PATCH 17/30] merge-recursive: Fix leaks of allocated renames and diff_filepairs
Elijah Newrenwrites: > get_renames() has always zero'ed out diff_queued_diff.nr while only > manually free'ing diff_filepairs that did not correspond to renames. > Further, it allocated struct renames that were tucked away in the > return string_list. Make sure all of these are deallocated when we > are done with them. > > Signed-off-by: Elijah Newren > --- > merge-recursive.c | 15 +-- > 1 file changed, 13 insertions(+), 2 deletions(-) > > diff --git a/merge-recursive.c b/merge-recursive.c > index 49710c0964..7a3402e50c 100644 > --- a/merge-recursive.c > +++ b/merge-recursive.c > @@ -1661,10 +1661,21 @@ static struct rename_info *handle_renames(struct > merge_options *o, > > static void cleanup_renames(struct rename_info *re_info) > { > - string_list_clear(re_info->head_renames, 0); > - string_list_clear(re_info->merge_renames, 0); > + const struct rename *re; > + int i; > > + for (i = 0; i < re_info->head_renames->nr; i++) { > + re = re_info->head_renames->items[i].util; > + diff_free_filepair(re->pair); > + } > + string_list_clear(re_info->head_renames, 1); > free(re_info->head_renames); > + > + for (i = 0; i < re_info->merge_renames->nr; i++) { > + re = re_info->merge_renames->items[i].util; > + diff_free_filepair(re->pair); > + } > + string_list_clear(re_info->merge_renames, 1); And this obviously will be helped by having another helper "cleanup_rename()" that does one of them, and call it twice from here.
Re: [PATCH 16/30] merge-recursive: Introduce new functions to handle rename logic
Elijah Newrenwrites: > +struct rename_info { > + struct string_list *head_renames; > + struct string_list *merge_renames; > +}; This type is added in order to allow the caller and the helper to communicate the findings in a single logical structure, instead of having to pass them as separate parameters, etc. If we anticipate that the information that needs to be passed will grow richer in later steps (or a follow-up series), such encapsulation makes a lot of sence. > +static struct rename_info *handle_renames(struct merge_options *o, > + struct tree *common, > + struct tree *head, > + struct tree *merge, > + struct string_list *entries, > + int *clean) > +{ > + struct rename_info *rei = xcalloc(1, sizeof(struct rename_info)); I however notice that there is only one caller of this helper at this step, and also at the end of this series. I suspect that it would probably be a better design to make "clean" the return value of this helper, and instead have the caller pass an uninitialised rename_info structure on its stack by address to be filled by the helper. > + rei->head_renames = get_renames(o, head, common, head, merge, entries); > + rei->merge_renames = get_renames(o, merge, common, head, merge, > entries); > + *clean = process_renames(o, rei->head_renames, rei->merge_renames); > + > + return rei; > +} > + > +static void cleanup_renames(struct rename_info *re_info) > +{ > + string_list_clear(re_info->head_renames, 0); > + string_list_clear(re_info->merge_renames, 0); > + > + free(re_info->head_renames); > + free(re_info->merge_renames); > + > + free(re_info); > +} > static struct object_id *stage_oid(const struct object_id *oid, unsigned > mode) > { > return (is_null_oid(oid) || mode == 0) ? NULL: (struct object_id *)oid; > @@ -1989,7 +2021,8 @@ int merge_trees(struct merge_options *o, > } > > if (unmerged_cache()) { > - struct string_list *entries, *re_head, *re_merge; > + struct string_list *entries; > + struct rename_info *re_info; > int i; > /* >* Only need the hashmap while processing entries, so > @@ -2003,9 +2036,7 @@ int merge_trees(struct merge_options *o, > get_files_dirs(o, merge); > > entries = get_unmerged(); > - re_head = get_renames(o, head, common, head, merge, entries); > - re_merge = get_renames(o, merge, common, head, merge, entries); > - clean = process_renames(o, re_head, re_merge); > + re_info = handle_renames(o, common, head, merge, entries, > ); > record_df_conflict_files(o, entries); > if (clean < 0) > goto cleanup; > @@ -2030,16 +2061,13 @@ int merge_trees(struct merge_options *o, > } > > cleanup: > - string_list_clear(re_merge, 0); > - string_list_clear(re_head, 0); > + cleanup_renames(re_info); > + > string_list_clear(entries, 1); > + free(entries); > > hashmap_free(>current_file_dir_set, 1); > > - free(re_merge); > - free(re_head); > - free(entries); > - > if (clean < 0) > return clean; > }
Re: [PATCH 15/30] merge-recursive: Move the get_renames() function
Elijah Newrenwrites: > I want to re-use some other functions in the file without moving those other > functions or dealing with a handful of annoying split function declarations > and definitions. > > Signed-off-by: Elijah Newren > --- It took me a while to figure out that you are basing this on top of a slightly older tip of 'master'. When rebasing on, or merging to, a newer codebase, these two lines > - diff_setup(); > - DIFF_OPT_SET(, RECURSIVE); > - DIFF_OPT_CLR(, RENAME_EMPTY); > - opts.detect_rename = DIFF_DETECT_RENAME; > ... > + diff_setup(); > + DIFF_OPT_SET(, RECURSIVE); > + DIFF_OPT_CLR(, RENAME_EMPTY); > + opts.detect_rename = DIFF_DETECT_RENAME; would need a bit of adjustment. By the way, checkpatch.pl complains about // C99 comments and binary operators missing SP on both ends, etc., on the entire series [*1*]. These look like small issues, but they are distracting enough to break concentration while reading the changes to spot places with real issues and places that can be improved, so cleaning them up early would help the final result to get better reviews. I won't reproduce all of them here, but here are a representable few. ERROR: spaces required around that '=' (ctx:VxV) #30: FILE: merge-recursive.c:1491: + for (i=0; inr; i++) { ^ ERROR: spaces required around that '<' (ctx:VxV) #30: FILE: merge-recursive.c:1491: + for (i=0; inr; i++) { ^ ERROR: "foo* bar" should be "foo *bar" #42: FILE: merge-recursive.c:1503: +static char* handle_path_level_conflicts(struct merge_options *o, WARNING: suspect code indent for conditional statements (8, 10) #80: FILE: merge-recursive.c:791: + if (o->call_depth || !was_tracked(path)) + return !dirty; Thanks. [Footnote] Just FYI, checkpatch.pl also notices these but it seems that our existing codebase already violates them in a major way, so I usually do not pay attention to these classes of complaints: ERROR: spaces required around that ':' (ctx:VxV) #30: FILE: merge-recursive.c:603: + unsigned add_turned_into_rename:1; ^ WARNING: quoted string split across lines #74: FILE: merge-recursive.c:1433: + output(o, 1, _("Refusing to lose untracked file at " + "%s, even though it's in the way."),
What's cooking in git.git (Nov 2017, #04; Tue, 14)
What's cooking in git.git (Nov 2017, #04; Tue, 14) -- 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. The tip of 'next' has been rebuilt on top of v2.15, while kicking a few topics back to 'pu'. 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"] * ad/5580-unc-tests-on-cygwin (2017-11-01) 1 commit (merged to 'next' on 2017-11-07 at 34fc479da2) + t5580: add Cygwin support UNC paths are also relevant in Cygwin builds and they are now tested just like Mingw builds. * ao/diff-populate-filespec-lstat-errorpath-fix (2017-10-29) 1 commit (merged to 'next' on 2017-11-07 at b03241e6e5) + diff: fix lstat() error handling in diff_populate_filespec() After an error from lstat(), diff_populate_filespec() function sometimes still went ahead and used invalid data in struct stat, which has been fixed. * bw/diff-opt-impl-to-bitfields (2017-11-01) 8 commits (merged to 'next' on 2017-11-07 at 8be78206ba) + diff: make struct diff_flags members lowercase + diff: remove DIFF_OPT_CLR macro + diff: remove DIFF_OPT_SET macro + diff: remove DIFF_OPT_TST macro + diff: remove touched flags + diff: add flag to indicate textconv was set via cmdline + diff: convert flags to be stored in bitfields + add, reset: use DIFF_OPT_SET macro to set a diff flag A single-word "unsigned flags" in the diff options is being split into a structure with many bitfields. * dk/libsecret-unlock-to-load-fix (2017-11-04) 1 commit (merged to 'next' on 2017-11-07 at 57d1d76c8c) + credential-libsecret: unlock locked secrets The credential helper for libsecret (in contrib/) has been improved to allow possibly prompting the end user to unlock secrets that are currently locked (otherwise the secrets may not be loaded). * jm/relnotes-2.15-typofix (2017-11-06) 1 commit (merged to 'next' on 2017-11-07 at 60fc937b62) + fix typos in 2.15.0 release notes Typofix. * jm/status-ignored-files-list (2017-10-31) 4 commits (merged to 'next' on 2017-11-07 at 682c74a2cb) + status: test ignored modes + status: document options to show matching ignored files + status: report matching ignored and normal untracked + status: add option to show ignored files differently Originally merged to 'next' on 2017-11-01 The set of paths output from "git status --ignored" was tied closely with its "--untracked=" option, but now it can be controlled more flexibly. Most notably, a directory that is ignored because it is listed to be ignored in the ignore/exclude mechanism can be handled differently from a directory that ends up to be ignored only because all files in it are ignored. * js/early-config (2017-11-03) 1 commit (merged to 'next' on 2017-11-07 at 9477c7c8ea) + setup: avoid double slashes when looking for HEAD Correct start-up sequence so that a repository could be placed immediately under the root directory again (which was broken at around Git 2.13). * js/mingw-full-version-in-resources (2017-11-01) 1 commit (merged to 'next' on 2017-11-07 at 3a256b5ddc) + mingw: include the full version information in the resources MinGW updates. * js/mingw-redirect-std-handles (2017-11-02) 3 commits (merged to 'next' on 2017-11-07 at 9af6a3dea0) + mingw: document the standard handle redirection + mingw: optionally redirect stderr/stdout via the same handle + mingw: add experimental feature to redirect standard handles MinGW updates. * js/wincred-empty-cred (2017-11-01) 2 commits (merged to 'next' on 2017-11-07 at 43d3fcc30a) + wincred: handle empty username/password correctly + t0302: check helper can handle empty credentials MinGW updates. * ks/mailmap (2017-11-03) 1 commit (merged to 'next' on 2017-11-07 at 46975637c7) + mailmap: use Kaartic Sivaraam's new address * rs/hex-to-bytes-cleanup (2017-11-01) 3 commits (merged to 'next' on 2017-11-07 at fac14770e1) + sha1_file: use hex_to_bytes() + http-push: use hex_to_bytes() + notes: move hex_to_bytes() to hex.c and export it Code cleanup. * sb/blame-config-doc (2017-11-06) 1 commit (merged to 'next' on 2017-11-07 at 0576cb452f) + config: document blame configuration Description of blame.{showroot,blankboundary,showemail,date} configuration variables have been added to "git config --help". * sg/travis-fixes (2017-11-02) 2 commits (merged to 'next' on 2017-11-07 at bbf39361b6) + travis-ci: don't build Git for the static analysis job + travis-ci: fix running P4 and Git LFS tests in Linux build jobs TravisCI build updates. -- [New
Re: [PATCH] t/lib-gpg: fix gpgconf stderr redirect to /dev/null
Todd Zullingerwrites: >> I wonder if this line in 3320 is doing what it meant to do: >> >>test_must_fail git notes merge z 2>&1 >out &&test_i18ngrep >> "Automatic notes merge failed" out &&grep -v "A notes merge into >> refs/notes/x is already in-progress in" out > > That's a fine question. I only grepped for 2>&1 >/dev/null. Dropping > /dev/null, as you did only turns up that test as an additional hit. > > I think, based on a very cursory reading of the test, that it's > intending to direct stderr and stdout to the file out. The test gets > lucky that the code in builtin/notes.c directs the error message to > stdout: > >printf(_("Automatic notes merge failed. Fix conflicts in %s and " > "commit the result with 'git notes merge --commit', or " > "abort the merge with 'git notes merge --abort'.\n"), > git_path(NOTES_MERGE_WORKTREE)); > > Perhaps that should be using fprintf(stderr, ...) instead? (And the > test redirection corrected as well, of course.) If that seems > correct, I can submit the trivial patch for that as well, while I'm on > the subject. The message goes to the standard output stream since it was introduced in 809f38c8 ("git notes merge: Manual conflict resolution, part 1/2", 2010-11-09) and 6abb3655 ("git notes merge: Manual conflict resolution, part 2/2", 2010-11-09). I do think it makes more sense to send it to the standard error stream, but just in case if the original author thinks of a reason why it shouldn't, let's summon Johan and ask his input. Thanks.
Re: man page for "git-worktree" is a bit confusing WRT "prune"
[+cc:Duy] On Mon, Nov 13, 2017 at 4:06 PM, Robert P. J. Daywrote: > On Mon, 13 Nov 2017, Eric Sunshine wrote: >> On Mon, Nov 13, 2017 at 9:48 AM, Robert P. J. Day >> wrote: >> > finally, the prune "--expire" option is truly confusing: >> > >> > --expire >> > With prune, only expire unused working trees older than . >> > >> > suddenly, we encounter the verb "expire", which means ... what? >> > how does "expiring" a worktree differ from "pruning" a worktree? >> > and what makes a worktree "unused"? the normal meaning of "unused" >> > is that you haven't, you know, *used* it lately. in this context, >> > though, does it mean deleted? and if it means deleted, what does >> > it mean for it to be older than some time if it's already gone? >> >> This dates back to the original behavior of automatically pruning >> administrative information for deleted worktrees. As discussed >> elsewhere in the document, a worktree may be placed on some >> removable device (USB drive, memory stick, etc.) or network share >> which isn't always mounted. The "expire time" provides such >> not-necessarily-mounted worktrees a grace period before being pruned >> automatically. > > how is this "expire time" measured? relative to what? i've looked > under .git/worktrees/, and i see a bunch of files defining > that worktree, but it's not clear how a worktree stores the relevant > time to be used for the determination of when a worktree "expires". According to Documentation/gitrepository-layout.txt: worktrees//gitdir:: A text file containing the absolute path back to the .git file that points to here. This is used to check if the linked repository has been manually removed and there is no need to keep this directory any more. The mtime of this file should be updated every time the linked repository is accessed. So, the expire time is relative to the mtime of the 'gitdir' file for that worktree. Presumably (according to the documentation excerpt), mtime of 'gitdir' is supposed to be updated each time the linked repository is accessed, however, I haven't found the code which does that (and it's been too long since I dealt which this code to remember where/if it is being done); in practice, I don't actually see the timestamp on 'gitdir' being updated, so that may be a bug/problem. > oh, and is it fair to assume that if a worktree is temporaily > missing, and is subsequently restored, the expire time counter is > reset? otherwise, it would get kind of weird. If we believe the documentation, then, as long as you've invoked some Git command recently enough in the restored worktree, it should be safe from pruning.
Re: [PATCH v2 6/6] Testing: provide tests requiring them with ellipses after SHA-1 values
Ann T Ropeawrites: > Where needed, we arrange for invocations of Git as if > >"-c core.printsha1ellipsis=true" > > had been specified on the command-line. This furnishes ellipses in the > output which then matches what is expected. > > Signed-off-by: Ann T Ropea > --- I am not a huge fan of exposing undocumented implementation details to the test scripts that much. There are three in t13xx series that mention GIT_CONFIG_PARAMETERS, but these are about testing the config mechanism itself. An end-user script would instead be doing "git -c var=val" for each invocation but this one is being lazy because it does not want to bother identifying which "git" invocation needs the treatment and also it does not want to keep maintaining it, which is understandable, but it feels dirty. This makes me wonder if core.printsha1ellipsis should really be a configuration variable in the first place. Wouldn't an environment variable be more appropriate? After all, the users of scripts that would be broken by this series would need to the same thing as what we do to our tests to keep them working while they update them. > v2: rename patch series & focus on removal of ellipses > t/t3040-subprojects-basic.sh | 12 > t/t4013-diff-various.sh | 12 > t/t9300-fast-import.sh | 12 > 3 files changed, 36 insertions(+) > > diff --git a/t/t3040-subprojects-basic.sh b/t/t3040-subprojects-basic.sh > index 0a4ff6d824a0..63b85bfdd4f9 100755 > --- a/t/t3040-subprojects-basic.sh > +++ b/t/t3040-subprojects-basic.sh > @@ -3,6 +3,18 @@ > test_description='Basic subproject functionality' > . ./test-lib.sh > > +# Some of the tests expect an ellipsis after the (abbreviated) > +# SHA-1 value. The code below results in Git being called with > +# "-c core.printsha1ellipsis=true" which satisfies those tests. > +do_print_sha1_ellipsis="'core.printsha1ellipsis=true'" > +if test -z "${GIT_CONFIG_PARAMETERS}" > +then > + GIT_CONFIG_PARAMETERS="${do_print_sha1_ellipsis}" > +else > + GIT_CONFIG_PARAMETERS="${GIT_CONFIG_PARAMETERS} > ${do_print_sha1_ellipsis}" > +fi > +export GIT_CONFIG_PARAMETERS > + > test_expect_success 'setup: create superproject' ' > : >Makefile && > git add Makefile && > diff --git a/t/t4013-diff-various.sh b/t/t4013-diff-various.sh > index c515e3e53fee..8ee14c7c6796 100755 > --- a/t/t4013-diff-various.sh > +++ b/t/t4013-diff-various.sh > @@ -7,6 +7,18 @@ test_description='Various diff formatting options' > > . ./test-lib.sh > > +# Some of the tests expect an ellipsis after the (abbreviated) > +# SHA-1 value. The code below results in Git being called with > +# "-c core.printsha1ellipsis=true" which satisfies those tests. > +do_print_sha1_ellipsis="'core.printsha1ellipsis=true'" > +if test -z "${GIT_CONFIG_PARAMETERS}" > +then > + GIT_CONFIG_PARAMETERS="${do_print_sha1_ellipsis}" > +else > + GIT_CONFIG_PARAMETERS="${GIT_CONFIG_PARAMETERS} > ${do_print_sha1_ellipsis}" > +fi > +export GIT_CONFIG_PARAMETERS > + > LF=' > ' > > diff --git a/t/t9300-fast-import.sh b/t/t9300-fast-import.sh > index d47560b6343d..6cc41b90dafa 100755 > --- a/t/t9300-fast-import.sh > +++ b/t/t9300-fast-import.sh > @@ -7,6 +7,18 @@ test_description='test git fast-import utility' > . ./test-lib.sh > . "$TEST_DIRECTORY"/diff-lib.sh ;# test-lib chdir's into trash > > +# Some of the tests expect an ellipsis after the (abbreviated) > +# SHA-1 value. The code below results in Git being called with > +# "-c core.printsha1ellipsis=true" which satisfies those tests. > +do_print_sha1_ellipsis="'core.printsha1ellipsis=true'" > +if test -z "${GIT_CONFIG_PARAMETERS}" > +then > + GIT_CONFIG_PARAMETERS="${do_print_sha1_ellipsis}" > +else > + GIT_CONFIG_PARAMETERS="${GIT_CONFIG_PARAMETERS} > ${do_print_sha1_ellipsis}" > +fi > +export GIT_CONFIG_PARAMETERS > + > verify_packs () { > for p in .git/objects/pack/*.pack > do
Re: [PATCH] t/lib-gpg: fix gpgconf stderr redirect to /dev/null
Santiago Torres wrote: Quick followup. The version that triggers this is at least 2.1.21[1]. I recall there was some wiggle room on minor versions before it. Thanks for digging that up! I had 2.1.13 at hand in a fedora-25 chroot which I used to build git recently, but I've not been able to coax the test failures from it, yet. I'll try a little more and with some different gnupg versions before I punt. If it's a small range of gnupg versions which fail badly when the GNUPGHOME dir is removed, then there's far less reason for git to do much more than make an effort to kill the agent. It seems like all the gnupg versions which may suffer from the bug also support gpgconf --kill, which would make any further change in the test to handle versions which lack the --kill option moot. -- Todd ~~ Our task must be to free ourselves from this prison by widening our circle of compassion to embrace all living creatures and the whole of nature in its beauty. -- Albert Einstein
Re: [PATCH v2 3/6] diff: diff_aligned_abbrev: remove ellipsis after abbreviated SHA-1 value
Ann T Ropeawrites: > Neither Git nor the user are in need of this (visual) aid anymore, but > we must offer a transition period. > > Also, fix a typo: "abbbreviated" ---> "abbreviated". > > Signed-off-by: Ann T Ropea > --- > v2: rename patch series & focus on removal of ellipses > diff.c | 69 > +- > 1 file changed, 39 insertions(+), 30 deletions(-) > > diff --git a/diff.c b/diff.c > index 0763e89263ef..9709dc37c6d7 100644 > --- a/diff.c > +++ b/diff.c > @@ -4902,41 +4902,50 @@ const char *diff_aligned_abbrev(const struct > object_id *oid, int len) > int abblen; > const char *abbrev; > > + /* Do we want all 40 hex characters? > + */ The oldest parts of the codebase (including diff.c) deviate from the rule in places, but in our multi-line comment, /* and */ occupy a line on their own. In this case, a single-line comment should be sufficient, though. > if (len == GIT_SHA1_HEXSZ) > return oid_to_hex(oid); > > - abbrev = diff_abbrev_oid(oid, len); > - abblen = strlen(abbrev); > - > - /* > - * In well-behaved cases, where the abbbreviated result is the > - * same as the requested length, append three dots after the > - * abbreviation (hence the whole logic is limited to the case > - * where abblen < 37); when the actual abbreviated result is a > - * bit longer than the requested length, we reduce the number > - * of dots so that they match the well-behaved ones. However, > - * if the actual abbreviation is longer than the requested > - * length by more than three, we give up on aligning, and add > - * three dots anyway, to indicate that the output is not the > - * full object name. Yes, this may be suboptimal, but this > - * appears only in "diff --raw --abbrev" output and it is not > - * worth the effort to change it now. Note that this would > - * likely to work fine when the automatic sizing of default > - * abbreviation length is used--we would be fed -1 in "len" in > - * that case, and will end up always appending three-dots, but > - * the automatic sizing is supposed to give abblen that ensures > - * uniqueness across all objects (statistically speaking). > + /* An abbreviated value is fine, possibly followed by an > + * ellipsis. >*/ Ditto. > - if (abblen < GIT_SHA1_HEXSZ - 3) { > - static char hex[GIT_MAX_HEXSZ + 1]; > - if (len < abblen && abblen <= len + 2) > - xsnprintf(hex, sizeof(hex), "%s%.*s", abbrev, > len+3-abblen, ".."); > - else > - xsnprintf(hex, sizeof(hex), "%s...", abbrev); > - return hex; > - } > + if (print_sha1_ellipsis) { > + abbrev = diff_abbrev_oid(oid, len); > + abblen = strlen(abbrev); > + > + /* > + * In well-behaved cases, where the abbreviated result is the > + * same as the requested length, append three dots after the > + * abbreviation (hence the whole logic is limited to the case > + * where abblen < 37); when the actual abbreviated result is a > + * bit longer than the requested length, we reduce the number > + * of dots so that they match the well-behaved ones. However, > + * if the actual abbreviation is longer than the requested > + * length by more than three, we give up on aligning, and add > + * three dots anyway, to indicate that the output is not the > + * full object name. Yes, this may be suboptimal, but this > + * appears only in "diff --raw --abbrev" output and it is not > + * worth the effort to change it now. Note that this would > + * likely to work fine when the automatic sizing of default > + * abbreviation length is used--we would be fed -1 in "len" in > + * that case, and will end up always appending three-dots, but > + * the automatic sizing is supposed to give abblen that ensures > + * uniqueness across all objects (statistically speaking). > + */ > + if (abblen < GIT_SHA1_HEXSZ - 3) { > + static char hex[GIT_MAX_HEXSZ + 1]; > + if (len < abblen && abblen <= len + 2) > + xsnprintf(hex, sizeof(hex), "%s%.*s", abbrev, > len+3-abblen, ".."); > + else > + xsnprintf(hex, sizeof(hex), "%s...", abbrev); > + return hex; > + } > > - return oid_to_hex(oid); > + return oid_to_hex(oid); > + } else { > + return diff_abbrev_oid(oid, len); > + } > } If I were writing this, I would have called diff_abbrev_oid() before checking
Re: [PATCH] t/lib-gpg: fix gpgconf stderr redirect to /dev/null
Junio C Hamano wrote: **Blush**. I should have caught this during the review. Thanks. I've written that code myself in the past and I am sure I will do it again. :) I wonder if this line in 3320 is doing what it meant to do: test_must_fail git notes merge z 2>&1 >out && test_i18ngrep "Automatic notes merge failed" out && grep -v "A notes merge into refs/notes/x is already in-progress in" out That's a fine question. I only grepped for 2>&1 >/dev/null. Dropping /dev/null, as you did only turns up that test as an additional hit. I think, based on a very cursory reading of the test, that it's intending to direct stderr and stdout to the file out. The test gets lucky that the code in builtin/notes.c directs the error message to stdout: printf(_("Automatic notes merge failed. Fix conflicts in %s and " "commit the result with 'git notes merge --commit', or " "abort the merge with 'git notes merge --abort'.\n"), git_path(NOTES_MERGE_WORKTREE)); Perhaps that should be using fprintf(stderr, ...) instead? (And the test redirection corrected as well, of course.) If that seems correct, I can submit the trivial patch for that as well, while I'm on the subject. -- Todd ~~ Chaos, panic, and disorder - my job is done here.
Re: [PATCH] t/lib-gpg: fix gpgconf stderr redirect to /dev/null
Todd Zullingerwrites: > In 29ff1f8f74 (t: lib-gpg: flush gpg agent on startup, 2017-07-20), a > call to gpgconf was added to kill the gpg-agent. The intention was to > ignore all output from the call, but the order of the redirection needs > to be switched to ensure that both stdout and stderr are redirected to > /dev/null. Without this, gpgconf from gnupg-2.0 releases would output > 'gpgconf: invalid option "--kill"' each time it was called. > > Signed-off-by: Todd Zullinger > --- > > I noticed that gpgconf produced error output for a number of tests on > CentOS/RHEL. As an example: > > *** t5534-push-signed.sh *** > gpgconf: invalid option "--kill" > > Looking at the code in lib-gpg.sh, it appeared the intention was to ignore > this > output. Reading through the review of the patch confirmed that feeling[1]. > The > current code gets caught by the subtleties of output redirection. (Who hasn't > been burned at some point by the difference between '2>&1 >/dev/null' and > '>/dev/null 2>&1' ? ;) **Blush**. I should have caught this during the review. Thanks. > Lastly, I also noticed that git-rebase.sh uses the same 2>&1 >/dev/null. I > suspect it's similarly not intentional: > > $ git grep -h -C4 '2>&1 >/dev/null' -- git-rebase.sh > apply_autostash () { > if test -f "$state_dir/autostash" > then > stash_sha1=$(cat "$state_dir/autostash") > if git stash apply $stash_sha1 2>&1 >/dev/null > then > echo "$(gettext 'Applied autostash.')" >&2 > else > git stash store -m "autostash" -q $stash_sha1 || > > I'll send a separate patch to adjust that code as well. If it were intentional, the caller of apply_autostash() must be expecting to see an error message from its standard output and prepared to do something interesting with it, which I do not see, so I agree that it is a typo. Thanks. I wonder if this line in 3320 is doing what it meant to do: test_must_fail git notes merge z 2>&1 >out && test_i18ngrep "Automatic notes merge failed" out && grep -v "A notes merge into refs/notes/x is already in-progress in" out
Re: [PATCH] Fix NO_LIBPCRE1_JIT to fully disable JIT
Charles Baileywrites: >> > But that we should take it anyway regardless of that since it'll *also* >> > work on Linux with your patch, and this logic makes some sense whereas >> > the other one clearly didn't and just worked by pure accident of some >> > toolchain semantics that I haven't figured out yet. >> >> That is curious and would be nice to know the answer to. > > The error that I was getting ... > My guess is that we are just exposing a pre-existing bug in our Solaris > build of libpcre. Sorry, my question was not clear. I think you already mentioned the above in the thread. What I was curious about was why Ævar was seeing that JIT disabled with NO_LIBPCRE1_JIT alone on his Linux setup, i.e. namely this part from his message: *But* for some reason you still get away with that on Linux. I don't know why, but I assume the compiler toolchain is more lax for some reason than on Solaris.n In any case, thanks for a fix; queued.
Re: [PATCH] link_alt_odb_entries: make empty input a noop
Joey Hesswrites: > Jeff King wrote: >> This should make Joey's immediate pain go away, though only by papering >> it over. I tend to agree that we shouldn't be looking at $PWD at all >> here. > > I've confirmed that Jeff's patch fixes the case I was having trouble with. Thanks, both.
[PATCH V2] config: add --expiry-date
From: HaarisDescription: This patch adds a new option to the config command. Uses flag --expiry-date as a data-type to covert date-strings to timestamps when reading from config files (GET). This flag is ignored on write (SET) because the date-string is stored in config without performing any normalization. Creates a few test cases and documentation since its a new feature. Motivation: A parse_expiry_date() function already existed for api calls, this patch simply allows the function to be used from the command line. Signed-off-by: Haaris --- Documentation/git-config.txt | 5 + builtin/config.c | 10 +- builtin/reflog.c | 14 ++ config.c | 9 + config.h | 1 + t/helper/test-date.c | 12 t/t1300-repo-config.sh | 30 ++ 7 files changed, 68 insertions(+), 13 deletions(-) Update: Added suggestions, documentation, relative time test case and test helper function to print out timestamps for comparison. Updated reflog.c to avoid function duplication. Sorry for duplicate messages, the other one didn't get threaded properly. diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt index 4edd09f..14da5fc 100644 --- a/Documentation/git-config.txt +++ b/Documentation/git-config.txt @@ -180,6 +180,11 @@ See also <>. value (but you can use `git config section.variable ~/` from the command line to let your shell do the expansion). +--expiry-date:: + `git config` will ensure that the output is converted from + a fixed or relative date-string to a timestamp. This option + has no effect when setting the value. + -z:: --null:: For all options that output values and/or keys, always diff --git a/builtin/config.c b/builtin/config.c index d13daee..afdb021 100644 --- a/builtin/config.c +++ b/builtin/config.c @@ -52,6 +52,7 @@ static int show_origin; #define TYPE_INT (1<<1) #define TYPE_BOOL_OR_INT (1<<2) #define TYPE_PATH (1<<3) +#define TYPE_EXPIRY_DATE (1<<4) static struct option builtin_config_options[] = { OPT_GROUP(N_("Config file location")), @@ -80,6 +81,7 @@ static struct option builtin_config_options[] = { OPT_BIT(0, "int", , N_("value is decimal number"), TYPE_INT), OPT_BIT(0, "bool-or-int", , N_("value is --bool or --int"), TYPE_BOOL_OR_INT), OPT_BIT(0, "path", , N_("value is a path (file or directory name)"), TYPE_PATH), + OPT_BIT(0, "expiry-date", , N_("value is an expiry date"), TYPE_EXPIRY_DATE), OPT_GROUP(N_("Other")), OPT_BOOL('z', "null", _null, N_("terminate values with NUL byte")), OPT_BOOL(0, "name-only", _values, N_("show variable names only")), @@ -159,6 +161,11 @@ static int format_config(struct strbuf *buf, const char *key_, const char *value return -1; strbuf_addstr(buf, v); free((char *)v); + } else if (types == TYPE_EXPIRY_DATE) { + timestamp_t t; + if(git_config_expiry_date(, key_, value_) < 0) + return -1; + strbuf_addf(buf, "%"PRItime, t); } else if (value_) { strbuf_addstr(buf, value_); } else { @@ -273,12 +280,13 @@ static char *normalize_value(const char *key, const char *value) if (!value) return NULL; - if (types == 0 || types == TYPE_PATH) + if (types == 0 || types == TYPE_PATH || types == TYPE_EXPIRY_DATE) /* * We don't do normalization for TYPE_PATH here: If * the path is like ~/foobar/, we prefer to store * "~/foobar/" in the config file, and to expand the ~ * when retrieving the value. +* Also don't do normalization for expiry dates. */ return xstrdup(value); if (types == TYPE_INT) diff --git a/builtin/reflog.c b/builtin/reflog.c index ab31a3b..2233725 100644 --- a/builtin/reflog.c +++ b/builtin/reflog.c @@ -416,16 +416,6 @@ static struct reflog_expire_cfg *find_cfg_ent(const char *pattern, size_t len) return ent; } -static int parse_expire_cfg_value(const char *var, const char *value, timestamp_t *expire) -{ - if (!value) - return config_error_nonbool(var); - if (parse_expiry_date(value, expire)) - return error(_("'%s' for '%s' is not a valid timestamp"), -value, var); - return 0; -} - /* expiry timer slot */ #define EXPIRE_TOTAL 01 #define EXPIRE_UNREACH 02 @@ -443,11 +433,11 @@ static int reflog_expire_config(const char *var, const char *value, void *cb) if (!strcmp(key, "reflogexpire")) { slot = EXPIRE_TOTAL; -
Re: [PATCH 04/30] directory rename detection: basic testcases
Stefan Bellerwrites: > On Fri, Nov 10, 2017 at 11:05 AM, Elijah Newren wrote: >> Signed-off-by: Elijah Newren >> ... >> +# B >> +# o >> +# / \ >> +# A o ? >> +# \ / >> +# o >> +# C >> + ... >> +# Testcase 1a, Basic directory rename. >> +# Commit A: z/{b,c} >> +# Commit B: y/{b,c} >> +# Commit C: z/{b,c,d,e/f} > > (minor thought:) > After rereading the docs above this is clear; I wonder if instead of A, B, C > a notation of Base, ours, theirs would be easier to understand? I had a similar thought, but as long as everything in this file is consistent, as we have that picture upfront, I am OK with it. FWIW, t1000 uses O (original--common ancestor) A and B, which was the notation commonly used in our codebase since the early days when we needed to call them with single letters. >> +test_expect_success '1a-setup: Simple directory rename detection' ' >> +test_expect_failure '1a-check: Simple directory rename detection' ' > > Thanks for splitting the setup and the check into two different test cases! > > >> + git checkout B^0 && > > Any reason for ^0 ? (to make clear it is a branch?) I think it is to make it clear that no matter what this test does (or fails to do), the branch B is *not* affected by it because we'd be playing on a detached head. >> +test_expect_success '1b-setup: Merge a directory with another' ' >> + git rm -rf . && >> + git clean -fdqx && >> + rm -rf .git && >> + git init && > > This is quite a strong statement to start a test with. Yes. If a test before this one did cd ../.. and forgot to come back, we'd be in trouble. If we want a fresh repository perhaps test-create-repo inside the trash repository may be a less evil option.
Re: [PATCH 04/30] directory rename detection: basic testcases
On Mon, Nov 13, 2017 at 5:21 PM, Stefan Bellerwrote: > On Mon, Nov 13, 2017 at 4:57 PM, Elijah Newren wrote: > (slightly dreaming:) > I wonder if we could teach our test suite to accept multiple test_done > or restart_tests or such to resurrect the clean slate. I'm dreaming now too; I would like that a lot more, although the separate test_create_repo for each case isn't too bad and should be a pretty mechanical switch. + test 3 -eq $(git ls-files -s | wc -l) && >>> >>> git ls-files >out && >>> test_line_count = 3 out && >>> > I am not saying it was a cargo-culting reaction, but rather relaying > a well discussed style issue to you. ;) Ah, gotcha. >> If you feel the return code of ls-files is important, perhaps I could >> just have a separate >>git ls-files -s >/dev/null && >> call before the others? > > I would prefer to not add any further calls; also (speaking generically) > this would bring in potential racing issues (what if the second ls-files > behaves differently than the first?) Makes sense. >> I'm not following. The "old" and "new" in the filenames were just >> there so a human reading the testcase could easily tell which >> filenames were related and involved in renames. There is no rename >> associated with d, so why would I have called it "old" or "new"? > > because a user may be impressed by gits pattern matching in the > rename detection: > > A: z/{oldb,oldc} > B: z/{newb,newc} > C: z/{oldb, oldc, oldd} > > Obviously A->B is z/{old->new}-* (not a directory rename, > but just patterns), so one might be tempted to expect newd > as the expectation. But that is nonsense(!?) Ah, now I see where you were going. Thanks for explaining. >> I think 2 is insanity. > > or the place where hooks/custom code shines. :) :)
Re: [PATCH 09/30] directory rename detection: testcases checking which side did the rename
On Mon, Nov 13, 2017 at 4:25 PM, Stefan Bellerwrote: > On Fri, Nov 10, 2017 at 11:05 AM, Elijah Newren wrote: >> +# Testcase 6b, Same rename done on both sides >> +# (Related to testcases 6c and 8e) >> +# Commit A: z/{b,c} >> +# Commit B: y/{b,c} >> +# Commit C: y/{b,c}, z/d > > Missing expected state Oops! >> +# Note: If we did directory rename detection here, we'd move z/d into y/, >> +# but C did that rename and still decided to put the file into z/, >> +# so we probably shouldn't apply directory rename detection for it. > > correct. Also we don't want to see a rename/rename conflict (obviously). Interestingly, (and this is unrelated to directory rename detection) merge-recursive.c has a RENAME_ONE_FILE_TO_ONE value exactly for the case where one file was renamed on both sides of history, but was renamed to the exact same thing on both sides. And in those cases, it turns what would otherwise be a rename/rename(1to2) conflict into essentially a RENAME_NORMAL case. So, we wouldn't have to worry about a rename/rename conflict anyway. >> +# Testcase 6e, Add/add from one-side >> +# Commit A: z/{b,c} >> +# Commit B: z/{b,c} (no change) >> +# Commit C: y/{b,c,d_1}, z/d_2 >> +# Expected: y/{b,c,d_1}, z/d_2 >> +# NOTE: Again, this seems obvious but just checking that the >> implementation >> +# doesn't "accidentally detect a rename" and give us y/{b,c} + >> +# add/add conflict on y/d_1 vs y/d_2. > > What is less obvious in all these cases is the "(no change)" part to me. > I would think that at least *something* changes in B in all cases above, maybe > add file u/r (un-related) to have the tree ids changed? > ("Less obvious" as in: we don't rely on the "no changes" part to make > the decision, > which sounds tempting so far) Yes, I could have introduced unrelated changes into the test, and my assumption is that the real world testcase would have such, it's just that in paring down to a minimal testcase I end up with a "no change" commit. >> test_done > > No conclusion box here, so my (misguided) suggestion: Yeah, the conclusion was actually in the summary. I could potentially restate it here: "Only apply implicit directory renames to directories if the _other_ side of history is the one doing the renaming" I can add that.
Re: [PATCH 03/30] merge-recursive: Add explanation for src_entry and dst_entry
Elijah Newrenwrites: > If I have to walk through the debugger and inspect the values found in > here in order to figure out their meaning, despite having known these > things inside and out some years back, then they probably need a comment > for the casual reader to explain their purpose. > > Signed-off-by: Elijah Newren > --- > merge-recursive.c | 22 ++ > 1 file changed, 22 insertions(+) > > diff --git a/merge-recursive.c b/merge-recursive.c > index 52521faf09..3526c8d0b8 100644 > --- a/merge-recursive.c > +++ b/merge-recursive.c > @@ -513,6 +513,28 @@ static void record_df_conflict_files(struct > merge_options *o, > > struct rename { > struct diff_filepair *pair; > + /* > + * Because I keep forgetting every few years what src_entry and > + * dst_entry are and have to walk through a debugger and puzzle > + * through it to remind myself... Very much appreciated. I recall having trouble reasoning about them myself, too (even though I admit I wasn't involved in the implementation of this part very much and know this code a lot less intimately than you do in the first place). > + * > + * If 'before' is renamed to 'after' then src_entry will contain > + * the versions of 'before' from the merge_base, HEAD, and MERGE in > + * stages 1, 2, and 3; dst_entry will contain the versions of > + * 'after' from the merge_base, HEAD, and MERGE in stages 1, 2, and > + * 3. Thus, we have a total of six modes and oids, though some > + * will be null. (Stage 0 is ignored; we're interested in handling > + * conflicts.) > + * > + * Since we don't turn on break-rewrites by default, neither > + * src_entry nor dst_entry can have all three of their stages have > + * non-null oids, meaning at most four of the six will be non-null. > + * Also, since this is a rename, both src_entry and dst_entry will > + * have at least one non-null oid, meaning at least two will be > + * non-null. Of the six oids, a typical rename will have three be > + * non-null. Only two implies a rename/delete, and four implies a > + * rename/add. > + */ > struct stage_data *src_entry; > struct stage_data *dst_entry; > unsigned processed:1;
Re: [PATCH 04/30] directory rename detection: basic testcases
On Mon, Nov 13, 2017 at 4:57 PM, Elijah Newrenwrote: > On Mon, Nov 13, 2017 at 2:04 PM, Stefan Beller wrote: >> On Fri, Nov 10, 2017 at 11:05 AM, Elijah Newren wrote: >> (minor thought:) >> After rereading the docs above this is clear; I wonder if instead of A, B, C >> a notation of Base, ours, theirs would be easier to understand? > > Sure, that'd be an easy change. > >>> +test_expect_success '1a-setup: Simple directory rename detection' ' >>> +test_expect_failure '1a-check: Simple directory rename detection' ' >> >> Thanks for splitting the setup and the check into two different test cases! >> >> >>> + git checkout B^0 && >> >> Any reason for ^0 ? (to make clear it is a branch?) > > Partially force-of-habit (did the same with t6036 and t6042), but it > seemed to have the nice feature of making debugging easier through > improved reproducability. In particular, if I had checked out B > rather than B^0 in the testcase and a merge succeeded when I didn't > expect it, then attempting to run the same commands gives me a > different starting point for the merge. By instead explicitly > checking out B^0, then even if the merge succeeds, someone who again > runs checkout B^0 will have the same starting point. Thanks for enlightening me. Makes sense. > >>> +test_expect_success '1b-setup: Merge a directory with another' ' >>> + git rm -rf . && >>> + git clean -fdqx && >>> + rm -rf .git && >>> + git init && >> >> This is quite a strong statement to start a test with. > > It was actually copy-paste from t6036, and achieved two purposes: > 1) Even if one test fails, subsequent ones continue running. (Had > lots of problems with this in t6036 years ago and just ended up with > those four steps) > 2) Someone who runs into a failing testcase has a _much_ easier time > figuring out what is going on with the testcase. I find it takes a > fair amount of time to figure out what's going on with other tests in > git's testsuite because of the presence of so many files completely > unrelated to the given test, which have simply accumulated from > previous tests. For many tests, that may be fine, but in particular > for t6036, t6042, and now t6043, since these are mostly about corner > cases that are hard enough to reason about, I didn't want the extra > distractions. I agree with these reasons to strongly want a clean slate. >> Nowadays we rather do >> >> test_when_finished "some command" && >> >> than polluting the follow up tests. But as you split up the previous test >> into 2 tests, it is not clear if this would bring any good. > > test_when_finished looks pretty cool; I didn't know about it. Thanks > for the pointer. Not sure if we want to use it here (if we do, we'd > only do so in the check tests rather than the setup ones). > >> Also these are four cleanup commands, I'd have expected fewer. >> Maybe just "rm -rf ." ? Or as we make a new repo for each test case, >> >> test_create_repo 1a && >> cd 1a >> >> in the first setup, and here we do >> test_create_repo 1b && >> cd 1b >> >> relying on test_done to cleanup everything afterwards? > > rm aborts for me with 'rm -rf .', but I could possibly make it 'rm -rf > * .* && git init .' > > The test_create_repo might not be so bad as long as every test used it > and put all files under it's own little repo. That is my current favorite, I would think. > It does create some > clutter, but it's at least somewhat managed. But the clutter is outside the repository under test, which may help with the situation. > I'm still a bit partial > to just totally cleaning it out, but if others would prefer, I can > switch. (slightly dreaming:) I wonder if we could teach our test suite to accept multiple test_done or restart_tests or such to resurrect the clean slate. > >>> + test 3 -eq $(git ls-files -s | wc -l) && >> >> git ls-files >out && >> test_line_count = 3 out && >> >> maybe? Piping output of git commands somewhere is an >> anti-pattern as we cannot examine the return code of ls-files in this case. > > Um...I guess that makes sense, if you assumed that I cared about the > return code of ls-files. As this is the test suite, we care about the return code of any git command, for current git as well as future-gits. > But it seems to make the tests with multiple > calls to ls-files in a row (of which there are many) considerably > uglier, so I'd personally prefer to avoid that. Also, why would I > care about the return code of ls-files? While I understand the rationale here and your examination of ls-files seems to indicate that we are ok doing it here, a lot of (test) code is taken for inspiration (copied, adapted) to other test cases. And most of the time we actually care if the return code is correct additionally to the actions performed, so I was shooting from the hip here. > Your suggestion made me curious, so I went looking. As far
Re: [PATCH 08/30] directory rename detection: files/directories in the way of some renames
On Mon, Nov 13, 2017 at 4:15 PM, Stefan Bellerwrote: > On Fri, Nov 10, 2017 at 11:05 AM, Elijah Newren wrote: >> +# Testcase 5c, Transitive rename would cause >> rename/rename/rename/add/add/add >> +# (Directory rename detection would result in transitive rename vs. >> +#rename/rename(1to2) and turn it into a rename/rename(1to3). Further, >> +#rename paths conflict with separate adds on the other side) >> +# (Related to testcases 3b and 7c) >> +# Commit A: z/{b,c}, x/d_1 >> +# Commit B: y/{b,c,d_2}, w/d_1 >> +# Commit C: z/{b,c,d_1,e}, w/d_3, y/d_4 >> +# Expected: A mess, but only a rename/rename(1to2)/add/add mess. Use the >> +# presence of y/d_4 in C to avoid doing transitive rename of >> +# x/d_1 -> z/d_1 -> y/d_1, so that the only paths we have at >> +# y/d are y/d_2 and y/d_4. We still do the move from z/e to >> y/e, >> +# though, because it doesn't have anything in the way. > > Missing the expected state, only the explanation is given. Yeah...it seemed so ugly to try to write down. As a possible sidenote, this testcase was actually guided by the final test of t6042, which is messy enough, but directory rename detection provides a little extra freedom to get a higher order conflict and make things a bit messier. It felt like it was a case where just leaving the expectation in code in the 5c-check was just easier and maybe even clearer. Should I add a comment to that effect, or would you really just prefer to see it spelled out? >> falling >> +# back to old handling. But, sadly, see testcases 8a and 8b. > > You seem to be hinting at these all the time. I think there were just multiple angles at which to approach those testcases. *shrug*
Re: [PATCH 3/4] progress: Fix progress meters when dealing with lots of work
Elijah Newrenwrites: > 2) Instead of counting pairs of source/dest files compared, just > count number of dest paths completed. (Thus, we wouldn't need a value > big enough to hold rename_dst_nr * rename_src_nr, just big enough to > hold rename_dst_nr). This sounds like the most sensible thing to do even if we do switch to larger integer size, but that is orthogonal to the main point of this series.
Re: [PATCH v2 0/9] sequencer: dont't fork git commit
Phillip Woodwrites: > Are you happy with the '--signoff' is handled (I didn't modify my > changes in the last iteration as you were still thinking about it)? I am not, but I am not unhappy, either. As I understand from your responses, it seems that the existing code had an untriggerable mess and the patch replaced it with another that is also untriggerable, and if that is the case, well, we do not have an immediate need to clean it up either way, so... ;-)
Re: [PATCH v1 1/4] fastindex: speed up index load through parallelization
Ben Peartwrites: > The proposed format for extensions (ie having both a header and a > footer with name and size) in the current patch already enables having > multiple extensions that can be parsed forward or backward. Any > extensions that would want to be parse-able in reverse would just all > need to be written one after the other after right before the trailing > SHA1 (and of course, include the proper footer). In other words, anything that wants to be scannable from the tail is welcome to reimplement the same validation structure used by IEOT to check the section specific sanity constraint, and this series is not interested in introducing a more general infrastructure to make it easy for code that want to find where each extension section in the file begins without pasing the body of the index. I find it somewhat unsatisfactory in that it is a fundamental change to allow jumping to the start of an extension section from the tail that can benefit any future codepath, and have expected a feature neutral extension whose sole purpose is to do so [*1*]. That way, extension sections whose names are all-caps can stay to be optional, even if they allow locating from the tail of the file. If you require them to implement the same validation struture as IEOT to perform section specific sanity constraint and also require them to be placed consecutively at the end, the reader MUST know about all such extensions---otherwise they cannot scan backwards and find ones that appear before an unknown but optional one. If you keep an extension section at the end whose sole purpose is to point at the beginning of extension sections, the reader can then scan forward as usual, skipping over unknown but optional ones, and reading your IEOT can merely be an user (and the first user) of that more generic feature that is futureproof, no?
Re: [PATCH 06/30] directory rename detection: testcases to avoid taking detection too far
On Mon, Nov 13, 2017 at 3:25 PM, Stefan Bellerwrote: > On Fri, Nov 10, 2017 at 11:05 AM, Elijah Newren wrote: >> +# Testcase 3a, Avoid implicit rename if involved as source on other side >> +# (Related to testcases 1c and 1f) >> +# Commit A: z/{b,c,d} >> +# Commit B: z/{b,c,d} (no change) > > This could also be an unrelated change such as adding w/e? Yes, precisely. Whenever I have a "no change" commit, the intent is that there may be unrelated changes involved, they've just been excluded from the testcase in order to make it minimal. >> +# Commit C: y/{b,c}, x/d >> +# Expected: y/{b,c}, x/d
Re: [PATCH 04/30] directory rename detection: basic testcases
On Mon, Nov 13, 2017 at 2:04 PM, Stefan Bellerwrote: > On Fri, Nov 10, 2017 at 11:05 AM, Elijah Newren wrote: > (minor thought:) > After rereading the docs above this is clear; I wonder if instead of A, B, C > a notation of Base, ours, theirs would be easier to understand? Sure, that'd be an easy change. >> +test_expect_success '1a-setup: Simple directory rename detection' ' >> +test_expect_failure '1a-check: Simple directory rename detection' ' > > Thanks for splitting the setup and the check into two different test cases! > > >> + git checkout B^0 && > > Any reason for ^0 ? (to make clear it is a branch?) Partially force-of-habit (did the same with t6036 and t6042), but it seemed to have the nice feature of making debugging easier through improved reproducability. In particular, if I had checked out B rather than B^0 in the testcase and a merge succeeded when I didn't expect it, then attempting to run the same commands gives me a different starting point for the merge. By instead explicitly checking out B^0, then even if the merge succeeds, someone who again runs checkout B^0 will have the same starting point. >> +test_expect_success '1b-setup: Merge a directory with another' ' >> + git rm -rf . && >> + git clean -fdqx && >> + rm -rf .git && >> + git init && > > This is quite a strong statement to start a test with. It was actually copy-paste from t6036, and achieved two purposes: 1) Even if one test fails, subsequent ones continue running. (Had lots of problems with this in t6036 years ago and just ended up with those four steps) 2) Someone who runs into a failing testcase has a _much_ easier time figuring out what is going on with the testcase. I find it takes a fair amount of time to figure out what's going on with other tests in git's testsuite because of the presence of so many files completely unrelated to the given test, which have simply accumulated from previous tests. For many tests, that may be fine, but in particular for t6036, t6042, and now t6043, since these are mostly about corner cases that are hard enough to reason about, I didn't want the extra distractions. but... > Nowadays we rather do > > test_when_finished "some command" && > > than polluting the follow up tests. But as you split up the previous test > into 2 tests, it is not clear if this would bring any good. test_when_finished looks pretty cool; I didn't know about it. Thanks for the pointer. Not sure if we want to use it here (if we do, we'd only do so in the check tests rather than the setup ones). > Also these are four cleanup commands, I'd have expected fewer. > Maybe just "rm -rf ." ? Or as we make a new repo for each test case, > > test_create_repo 1a && > cd 1a > > in the first setup, and here we do > test_create_repo 1b && > cd 1b > > relying on test_done to cleanup everything afterwards? rm aborts for me with 'rm -rf .', but I could possibly make it 'rm -rf * .* && git init .' The test_create_repo might not be so bad as long as every test used it and put all files under it's own little repo. It does create some clutter, but it's at least somewhat managed. I'm still a bit partial to just totally cleaning it out, but if others would prefer, I can switch. >> + test 3 -eq $(git ls-files -s | wc -l) && > > git ls-files >out && > test_line_count = 3 out && > > maybe? Piping output of git commands somewhere is an > anti-pattern as we cannot examine the return code of ls-files in this case. Um...I guess that makes sense, if you assumed that I cared about the return code of ls-files. But it seems to make the tests with multiple calls to ls-files in a row (of which there are many) considerably uglier, so I'd personally prefer to avoid that. Also, why would I care about the return code of ls-files? Your suggestion made me curious, so I went looking. As far as I can tell, the return code of ls-files is always 0 unless you both specify both --error-unmatch and one or more paths, neither of which I did. Am I missing something? If you feel the return code of ls-files is important, perhaps I could just have a separate git ls-files -s >/dev/null && call before the others? >> + test $(git rev-parse HEAD:y/b) = $(git rev-parse A:z/b) && >> + test $(git rev-parse HEAD:y/c) = $(git rev-parse A:z/c) && >> + test $(git rev-parse HEAD:y/d) = $(git rev-parse A:x/d) && > > Speaking of these, there are quite a lot of invocations of rev-parse, > though it looks clean; recently Junio had some good ideas regarding a > similar test[1]. > So maybe > > git rev-parse >actual \ > HEAD:y/b HEAD:y/c HEAD:y/d && > git rev-parse >expect \ > A:z/bA:z/cA:x/d && > test_cmp expect actual > > Not sure if that is any nicer, but has fewer calls. Sure, I can switch it over. >> + test_must_fail git rev-parse HEAD:x/d && >> + test_must_fail git rev-parse HEAD:z/d && >> +
Re: [PATCH 10/30] directory rename detection: more involved edge/corner testcases
On Fri, Nov 10, 2017 at 11:05 AM, Elijah Newrenwrote: > +# In my opinion, testcases that are difficult to understand from this > +# section is due to difficulty in the testcase rather than the directory > +# renaming (similar to how t6042 and t6036 have difficult resolutions due > +# to the problem setup itself being complex). And I don't think the > +# error messages are a problem. "In my opinion" ... sounds like commit message? > +# On the other hand, the testcases in section 8 worry me slightly more... The aforementioned section 8... :) > +# Testcase 7a, rename-dir vs. rename-dir (NOT split evenly) PLUS > add-other-file > +# Commit A: z/{b,c} > +# Commit B: y/{b,c} > +# Commit C: w/b, x/c, z/d > +# Expected: y/d, CONFLICT(rename/rename for both z/b and z/c) > +# NOTE: There's a rename of z/ here, y/ has more renames, so z/d -> y/d. But the creator of C intended to have z/d, not {w,x}/d, and as {w,x} == y, I am not sure I like this result. (I have no concrete counter example, just messy logic) > +# Testcase 7b, rename/rename(2to1), but only due to transitive rename > +# (Related to testcase 1d) > +# Commit A: z/{b,c}, x/d_1, w/d_2 > +# Commit B: y/{b,c,d_2}, x/d_1 > +# Commit C: z/{b,c,d_1},w/d_2 > +# Expected: y/{b,c}, CONFLICT(rename/rename(2to1): x/d_1, w/d_2 -> y_d) Makes sense. > +# Testcase 7c, rename/rename(1to...2or3); transitive rename may add > complexity > +# (Related to testcases 3b and 5c) > +# Commit A: z/{b,c}, x/d > +# Commit B: y/{b,c}, w/d > +# Commit C: z/{b,c,d} > +# Expected: y/{b,c}, CONFLICT(x/d -> w/d vs. y/d) CONFLICT(x/d -> y/d vs w/d) ? > +# NOTE: z/ was renamed to y/ so we do not want to report > +# either CONFLICT(x/d -> w/d vs. z/d) > +# or CONFLiCT x/d -> w/d vs. y/d vs. z/d) "neither ... nor" instead of "not either or"? > +# Testcase 7d, transitive rename involved in rename/delete; how is it > reported? > +# (Related somewhat to testcases 5b and 8d) > +# Commit A: z/{b,c}, x/d > +# Commit B: y/{b,c} > +# Commit C: z/{b,c,d} > +# Expected: y/{b,c}, CONFLICT(delete x/d vs rename to y/d) > +# NOTE: z->y so NOT CONFLICT(delete x/d vs rename to z/d) > +# Testcase 7e, transitive rename in rename/delete AND dirs in the way > +# (Very similar to 'both rename source and destination involved in D/F > conflict' from t6022-merge-rename.sh) > +# (Also related to testcases 9c and 9d) > +# Commit A: z/{b,c}, x/d_1 > +# Commit B: y/{b,c,d/g}, x/d/f > +# Commit C: z/{b,c,d_1} > +# Expected: rename/delete(x/d_1->y/d_1 vs. None) + D/F conflict on y/d > +# y/{b,c,d/g}, y/d_1~C^0, x/d/f > +# NOTE: x/d/f may be slightly confusing here. x/d_1 -> z/d_1 implies > +# there is a directory rename from x/ -> z/, performed by commit C. > +# However, on the side of commit B, it renamed z/ -> y/, thus > +# making a rename from x/ -> z/ when it was getting rid of z/ seems > +# non-sensical. Further, putting x/d/f into y/d/f also doesn't > +# make a lot of sense because commit B did the renaming of z to y > +# and it created x/d/f, and it clearly made these things separate, > +# so it doesn't make much sense to push these together. This is confusing.
Re: [PATCH 09/30] directory rename detection: testcases checking which side did the rename
On Fri, Nov 10, 2017 at 11:05 AM, Elijah Newrenwrote: > Signed-off-by: Elijah Newren > --- > t/t6043-merge-rename-directories.sh | 283 > > 1 file changed, 283 insertions(+) > > diff --git a/t/t6043-merge-rename-directories.sh > b/t/t6043-merge-rename-directories.sh > index d15153c652..157299105f 100755 > --- a/t/t6043-merge-rename-directories.sh > +++ b/t/t6043-merge-rename-directories.sh > @@ -1053,4 +1053,287 @@ test_expect_failure '5d-check: Directory/file/file > conflict due to directory ren > # back to old handling. But, sadly, see testcases 8a and 8b. > ### > > + > +### > +# SECTION 6: Same side of the merge was the one that did the rename > +# > +# It may sound obvious that you only want to apply implicit directory > +# renames to directories if the _other_ side of history did the renaming. > +# If you did make an implementation that didn't explicitly enforce this > +# rule, the majority of cases that would fall under this section would > +# also be solved by following the rules from the above sections. But > +# there are still a few that stick out, so this section covers them just > +# to make sure we also get them right. > +### > + > +# Testcase 6a, Tricky rename/delete > +# Commit A: z/{b,c,d} > +# Commit B: z/b > +# Commit C: y/{b,c}, z/d > +# Expected: y/b, CONFLICT(rename/delete, z/c -> y/c vs. NULL) > +# Note: We're just checking here that the rename of z/b and z/c to put > +# them under y/ doesn't accidentally catch z/d and make it look like > +# it is also involved in a rename/delete conflict. > + > + > +# Testcase 6b, Same rename done on both sides > +# (Related to testcases 6c and 8e) > +# Commit A: z/{b,c} > +# Commit B: y/{b,c} > +# Commit C: y/{b,c}, z/d Missing expected state > +# Note: If we did directory rename detection here, we'd move z/d into y/, > +# but C did that rename and still decided to put the file into z/, > +# so we probably shouldn't apply directory rename detection for it. correct. Also we don't want to see a rename/rename conflict (obviously). If we have Commit A: z/{b_1,c} Commit B: y/{b_2,c} Commit C: y/{b_3,c}, z/d then we'd produce a standard file merge (which may or may not result in conflict, depending on touched lines) for y/b_{try-resolve} > + > +# Testcase 6c, Rename only done on same side > +# (Related to testcases 6b and 8e) > +# Commit A: z/{b,c} > +# Commit B: z/{b,c} (no change) > +# Commit C: y/{b,c}, z/d > +# Expected: y/{b,c}, z/d > +# NOTE: Seems obvious, but just checking that the implementation doesn't > +# "accidentally detect a rename" and give us y/{b,c,d}. makes sense. > + > +# Testcase 6d, We don't always want transitive renaming > +# (Related to testcase 1c) > +# Commit A: z/{b,c}, x/d > +# Commit B: z/{b,c}, x/d (no change) > +# Commit C: y/{b,c}, z/d > +# Expected: y/{b,c}, z/d > +# NOTE: Again, this seems obvious but just checking that the implementation > +# doesn't "accidentally detect a rename" and give us y/{b,c,d}. makes sense, too. > +# Testcase 6e, Add/add from one-side > +# Commit A: z/{b,c} > +# Commit B: z/{b,c} (no change) > +# Commit C: y/{b,c,d_1}, z/d_2 > +# Expected: y/{b,c,d_1}, z/d_2 > +# NOTE: Again, this seems obvious but just checking that the implementation > +# doesn't "accidentally detect a rename" and give us y/{b,c} + > +# add/add conflict on y/d_1 vs y/d_2. What is less obvious in all these cases is the "(no change)" part to me. I would think that at least *something* changes in B in all cases above, maybe add file u/r (un-related) to have the tree ids changed? ("Less obvious" as in: we don't rely on the "no changes" part to make the decision, which sounds tempting so far) > test_done No conclusion box here, so my (misguided) suggestion: If "No change" occurs, just take the other side. ;)
Re: [PATCH 08/30] directory rename detection: files/directories in the way of some renames
On Fri, Nov 10, 2017 at 11:05 AM, Elijah Newrenwrote: > Signed-off-by: Elijah Newren > --- > t/t6043-merge-rename-directories.sh | 303 > > 1 file changed, 303 insertions(+) > > diff --git a/t/t6043-merge-rename-directories.sh > b/t/t6043-merge-rename-directories.sh > index ec054b210a..d15153c652 100755 > --- a/t/t6043-merge-rename-directories.sh > +++ b/t/t6043-merge-rename-directories.sh > @@ -750,4 +750,307 @@ test_expect_success '4a-check: Directory split, with > original directory still pr > # detection.) But, sadly, see testcase 8b. > ### > > + > +### > +# SECTION 5: Files/directories in the way of subset of to-be-renamed paths > +# > +# Implicitly renaming files due to a detected directory rename could run > +# into problems if there are files or directories in the way of the paths > +# we want to rename. Explore such cases in this section. > +### > + > +# Testcase 5a, Merge directories, other side adds files to original and > target > +# Commit A: z/{b,c}, y/d > +# Commit B: z/{b,c,e_1,f}, y/{d,e_2} > +# Commit C: y/{b,c,d} > +# Expected: z/e_1, y/{b,c,d,e_2,f} + CONFLICT warning > +# NOTE: While directory rename detection is active here causing z/f to > +# become y/f, we did not apply this for z/e_1 because that would > +# give us an add/add conflict for y/e_1 vs y/e_2. This problem with > +# this add/add, is that both versions of y/e are from the same side > +# of history, giving us no way to represent this conflict in the > +# index. Makes sense. > +# Testcase 5b, Rename/delete in order to get add/add/add conflict > +# (Related to testcase 8d; these may appear slightly inconsistent to users; > +#Also related to testcases 7d and 7e) > +# Commit A: z/{b,c,d_1} > +# Commit B: y/{b,c,d_2} > +# Commit C: z/{b,c,d_1,e}, y/d_3 > +# Expected: y/{b,c,e}, CONFLICT(add/add: y/d_2 vs. y/d_3) > +# NOTE: If z/d_1 in commit C were to be involved in dir rename detection, > as > +# we normaly would since z/ is being renamed to y/, then this would > be > +# a rename/delete (z/d_1 -> y/d_1 vs. deleted) AND an add/add/add > +# conflict of y/d_1 vs. y/d_2 vs. y/d_3. Add/add/add is not > +# representable in the index, so the existence of y/d_3 needs to > +# cause us to bail on directory rename detection for that path, > falling > +# back to git behavior without the directory rename detection. > + > +# Testcase 5c, Transitive rename would cause rename/rename/rename/add/add/add > +# (Directory rename detection would result in transitive rename vs. > +#rename/rename(1to2) and turn it into a rename/rename(1to3). Further, > +#rename paths conflict with separate adds on the other side) > +# (Related to testcases 3b and 7c) > +# Commit A: z/{b,c}, x/d_1 > +# Commit B: y/{b,c,d_2}, w/d_1 > +# Commit C: z/{b,c,d_1,e}, w/d_3, y/d_4 > +# Expected: A mess, but only a rename/rename(1to2)/add/add mess. Use the > +# presence of y/d_4 in C to avoid doing transitive rename of > +# x/d_1 -> z/d_1 -> y/d_1, so that the only paths we have at > +# y/d are y/d_2 and y/d_4. We still do the move from z/e to y/e, > +# though, because it doesn't have anything in the way. Missing the expected state, only the explanation is given. > +# Testcase 5d, Directory/file/file conflict due to directory rename > +# Commit A: z/{b,c} > +# Commit B: y/{b,c,d_1} > +# Commit C: z/{b,c,d_2,f}, y/d/e > +# Expected: y/{b,c,d/e,f}, z/d_2, CONFLICT(file/directory), y/d_1~HEAD > +# Note: The fact that y/d/ exists in C makes us bail on directory rename > +# detection for z/d_2, but that doesn't prevent us from applying the > +# directory rename detection for z/f -> y/f. Makes sense. > + > +### > +# Rules suggested by section 5: > +# > +# If a subset of to-be-renamed files have a file or directory in the way, > +# "turn off" the directory rename for those specific sub-paths, Makes sense. > falling > +# back to old handling. But, sadly, see testcases 8a and 8b. You seem to be hinting at these all the time.
Re: [PATCH 07/30] directory rename detection: partially renamed directory testcase/discussion
On Fri, Nov 10, 2017 at 11:05 AM, Elijah Newrenwrote: > Signed-off-by: Elijah Newren > --- > t/t6043-merge-rename-directories.sh | 100 > > 1 file changed, 100 insertions(+) > > diff --git a/t/t6043-merge-rename-directories.sh > b/t/t6043-merge-rename-directories.sh > index 021513ec00..ec054b210a 100755 > --- a/t/t6043-merge-rename-directories.sh > +++ b/t/t6043-merge-rename-directories.sh > @@ -650,4 +650,104 @@ test_expect_success '3b-check: Avoid implicit rename if > involved as source on cu > # of a rename on either side of a merge. > ### > > + > +### > +# SECTION 4: Partially renamed directory; still exists on both sides of merge > +# > +# What if we were to attempt to do directory rename detection when someone > +# "mostly" moved a directory but still left some files around, or, > +# equivalently, fully renamed a directory in one commmit and then recreated > +# that directory in a later commit adding some new files and then tried to > +# merge? > +# > +# It's hard to divine user intent in these cases, because you can make an > +# argument that, depending on the intermediate history of the side being > +# merged, that some users will want files in that directory to > +# automatically be detected and renamed, while users with a different > +# intermediate history wouldn't want that rename to happen. > +# > +# I think that it is best to simply not have directory rename detection > +# apply to such cases. My reasoning for this is four-fold: (1) it's > +# easiest for users in general to figure out what happened if we don't > +# apply directory rename detection in any such case, (2) it's an easy rule > +# to explain ["We don't do directory rename detection if the directory > +# still exists on both sides of the merge"], (3) we can get some hairy > +# edge/corner cases that would be really confusing and possibly not even > +# representable in the index if we were to even try, and [related to 3] (4) > +# attempting to resolve this issue of divining user intent by examining > +# intermediate history goes against the spirit of three-way merges and is a > +# path towards crazy corner cases that are far more complex than what we're > +# already dealing with. This last paragraph ("I think") sounds like part of a commit message, rather than a note inside a testing script. Not sure if I recommend moving this text into the commit message. > +# This section contains a test for this partially-renamed-directory case. > +### > + > +# Testcase 4a, Directory split, with original directory still present > +# (Related to testcase 1f) > +# Commit A: z/{b,c,d,e} > +# Commit B: y/{b,c,d}, z/e > +# Commit C: z/{b,c,d,e,f} > +# Expected: y/{b,c,d}, z/{e,f} > +# NOTE: Even though most files from z moved to y, we don't want f to > follow. Makes sense. > +### > +# Rules suggested by section 4: > +# > +# Directory-rename-detection should be turned off for any directories (as > +# a source for renames) that exist on both sides of the merge. (The "as > +# a source for renames" clarification is due to cases like 1c where > +# the target directory exists on both sides and we do want the rename > +# detection.) But, sadly, see testcase 8b. Looking forward for that test case.
Re: [PATCH 02/30] merge-recursive: Fix logic ordering issue
On Mon, Nov 13, 2017 at 3:39 PM, Elijah Newrenwrote: > On Mon, Nov 13, 2017 at 2:12 PM, Stefan Beller wrote: >> I wanted to debug a very similar issue today just after reviewing this >> series, see >> https://public-inbox.org/git/743acc29-85bb-3773-b6a0-68d4a0b8f...@ispras.ru/ > > Oh, bleh. That's not a D/F conflict at all, it's the code assuming > there's a D/F conflict because the entry it is processing ("sub") is a > submodule rather than a file, and it panics when it sees "a directory > in the way" -- a directory that just so happens to be named "sub" and > which is in fact the desired submodule, meaning that the working > directory is already good and needs no changes. yup, I came to find the same snippet of code to be the offender, I just haven't figured out how to fix this bug. Thanks for taking a look! As you have a lot of fresh knowledge in the merge-recursive case currently, how would we approach the fix here? (there is a test available at remotes/origin/sb/test-cherry-pick-submodule-getting-in-a-way) > In this case, the relevant code from merge-recursive.c is the following: > > /* Case B: Added in one. */ > /* [nothing|directory] -> ([nothing|directory], file) */ > > if (dir_in_way(path, !o->call_depth, >S_ISGITLINK(a_mode))) { > char *new_path = unique_path(o, path, add_branch); > clean_merge = 0; > output(o, 1, _("CONFLICT (%s): There is a directory with > name %s in %s. " >"Adding %s as %s"), >conf, path, other_branch, path, new_path); > > Note that the comment even explicitly assumes the newly added entry is > a file. We should expect there to be a directory present (the > submodule being added), but the code doesn't have a check for that. > The S_ISGITLINK(a_mode) makes you think it has special handling for > the submodule case, but that's for the reverse situation (the > submodule isn't yet present in the working copy, it came from the > other side of history, but there is an empty directory present). oh :/
Re: [PATCH 02/30] merge-recursive: Fix logic ordering issue
On Mon, Nov 13, 2017 at 2:12 PM, Stefan Bellerwrote: > I wanted to debug a very similar issue today just after reviewing this > series, see > https://public-inbox.org/git/743acc29-85bb-3773-b6a0-68d4a0b8f...@ispras.ru/ Oh, bleh. That's not a D/F conflict at all, it's the code assuming there's a D/F conflict because the entry it is processing ("sub") is a submodule rather than a file, and it panics when it sees "a directory in the way" -- a directory that just so happens to be named "sub" and which is in fact the desired submodule, meaning that the working directory is already good and needs no changes. In this case, the relevant code from merge-recursive.c is the following: /* Case B: Added in one. */ /* [nothing|directory] -> ([nothing|directory], file) */ if (dir_in_way(path, !o->call_depth, S_ISGITLINK(a_mode))) { char *new_path = unique_path(o, path, add_branch); clean_merge = 0; output(o, 1, _("CONFLICT (%s): There is a directory with name %s in %s. " "Adding %s as %s"), conf, path, other_branch, path, new_path); Note that the comment even explicitly assumes the newly added entry is a file. We should expect there to be a directory present (the submodule being added), but the code doesn't have a check for that. The S_ISGITLINK(a_mode) makes you think it has special handling for the submodule case, but that's for the reverse situation (the submodule isn't yet present in the working copy, it came from the other side of history, but there is an empty directory present).
Re: [PATCH 06/30] directory rename detection: testcases to avoid taking detection too far
On Fri, Nov 10, 2017 at 11:05 AM, Elijah Newrenwrote: > Signed-off-by: Elijah Newren > --- > t/t6043-merge-rename-directories.sh | 137 > > 1 file changed, 137 insertions(+) > > diff --git a/t/t6043-merge-rename-directories.sh > b/t/t6043-merge-rename-directories.sh > index 00811f512a..021513ec00 100755 > --- a/t/t6043-merge-rename-directories.sh > +++ b/t/t6043-merge-rename-directories.sh > @@ -513,4 +513,141 @@ test_expect_success '2b-check: Directory split into two > on one side, with equal > # messages are handled correctly. > ### > > + > +### > +# SECTION 3: Path in question is the source path for some rename already > +# > +# Combining cases from Section 1 and trying to handle them could lead to > +# directory renaming detection being over-applied. So, this section > +# provides some good testcases to check that the implementation doesn't go > +# too far. > +### > + > +# Testcase 3a, Avoid implicit rename if involved as source on other side > +# (Related to testcases 1c and 1f) > +# Commit A: z/{b,c,d} > +# Commit B: z/{b,c,d} (no change) This could also be an unrelated change such as adding w/e? > +# Commit C: y/{b,c}, x/d > +# Expected: y/{b,c}, x/d > + > +# Testcase 3b, Avoid implicit rename if involved as source on other side > +# (Related to testcases 5c and 7c, also kind of 1e and 1f) > +# Commit A: z/{b,c,d} > +# Commit B: y/{b,c}, x/d > +# Commit C: z/{b,c}, w/d > +# Expected: y/{b,c}, CONFLICT:(z/d -> x/d vs. w/d) > +# NOTE: We're particularly checking that since z/d is already involved as > +# a source in a file rename on the same side of history, that we > don't > +# get it involved in directory rename detection. If it were, we > might > +# end up with CONFLICT:(z/d -> y/d vs. x/d vs. w/d), i.e. a > +# rename/rename/rename(1to3) conflict, which is just weird. Makes sense. >
Re: [PATCH 05/30] directory rename detection: directory splitting testcases
On Fri, Nov 10, 2017 at 11:05 AM, Elijah Newrenwrote: > Signed-off-by: Elijah Newren > --- > t/t6043-merge-rename-directories.sh | 125 > > 1 file changed, 125 insertions(+) > > diff --git a/t/t6043-merge-rename-directories.sh > b/t/t6043-merge-rename-directories.sh > index b737b0a105..00811f512a 100755 > --- a/t/t6043-merge-rename-directories.sh > +++ b/t/t6043-merge-rename-directories.sh > @@ -388,4 +388,129 @@ test_expect_failure '1f-check: Split a directory into > two other directories' ' > # in section 2, plus testcases 3a and 4a. > ### > > + > +### > +# SECTION 2: Split into multiple directories, with equal number of paths > +# > +# Explore the splitting-a-directory rules a bit; what happens in the > +# edge cases? > +# > +# Note that there is a closely related case of a directory not being > +# split on either side of history, but being renamed differently on > +# each side. See testcase 8e for that. > +### > + > +# Testcase 2a, Directory split into two on one side, with equal numbers of > paths > +# Commit A: z/{b,c} > +# Commit B: y/b, w/c > +# Commit C: z/{b,c,d} > +# Expected: y/b, w/c, z/d, with warning about z/ -> (y/ vs. w/) conflict > + test_i18ngrep "CONFLICT.*directory rename split" out > +# Testcase 2b, Directory split into two on one side, with equal numbers of > paths > +# Commit A: z/{b,c} > +# Commit B: y/b, w/c > +# Commit C: z/{b,c}, x/d > +# Expected: y/b, w/c, x/d; No warning about z/ -> (y/ vs. w/) conflict This makes sense. > + > +### > +# Rules suggested by section 2: > +# > +# None; the rule was already covered in section 1. These testcases are > +# here just to make sure the conflict resolution and necessary warning > +# messages are handled correctly. > +### okay, then I'll go back to 1. and discuss "the number of files as a hint where to rename it to" there
Re: [PATCH 03/30] merge-recursive: Add explanation for src_entry and dst_entry
On Mon, Nov 13, 2017 at 2:57 PM, Elijah Newrenwrote: > > Perhaps: > > If 'before' is renamed to 'after' then src_entry will contain > the versions of 'before' from the merge_base, HEAD, and MERGE in > stages 1, 2, and 3; and dst_entry will contain the respective versions of > 'after' in corresponding locations. Thus, we have a total of six modes > and oids, though some will be null. (Stage 0 is ignored; we're interested > in handling conflicts.) I find that much easier to read, though I am biased with prior knowledge now. ;)
Re: [PATCH] t/lib-gpg: fix gpgconf stderr redirect to /dev/null
Quick followup. The version that triggers this is at least 2.1.21[1]. I recall there was some wiggle room on minor versions before it. Thanks! -Santiago. [1] https://dev.gnupg.org/T3218 On Mon, Nov 13, 2017 at 06:02:02PM -0500, Santiago Torres wrote: > > > Were the ENOENT errors you encountered in running the tests multiple times > > easy to reproduce? > > If you had the right gpg2, it should be easy to repro with just re-running. > > > If so, I can certainly try to reproduce them and then > > run the tests with --reload in place of --kill to gpgconf. If that worked > > across the various gnupg 2.x releases, it would be a simple enough change to > > make as a follow-up. > > Let me dig up the exact versions. IIRC it was somewhere between 2.1.0 and > 2.2.x > or so. I think somewhere within the patch re-rolls I had the exact versions. > > Cheers! > -Santiago. signature.asc Description: PGP signature
Re: [PATCH] t/lib-gpg: fix gpgconf stderr redirect to /dev/null
> Were the ENOENT errors you encountered in running the tests multiple times > easy to reproduce? If you had the right gpg2, it should be easy to repro with just re-running. > If so, I can certainly try to reproduce them and then > run the tests with --reload in place of --kill to gpgconf. If that worked > across the various gnupg 2.x releases, it would be a simple enough change to > make as a follow-up. Let me dig up the exact versions. IIRC it was somewhere between 2.1.0 and 2.2.x or so. I think somewhere within the patch re-rolls I had the exact versions. Cheers! -Santiago. signature.asc Description: PGP signature
Re: [PATCH 03/30] merge-recursive: Add explanation for src_entry and dst_entry
On Mon, Nov 13, 2017 at 1:06 PM, Stefan Bellerwrote: > On Fri, Nov 10, 2017 at 11:05 AM, Elijah Newren wrote: >> + /* >> +* Because I keep forgetting every few years what src_entry and >> +* dst_entry are and have to walk through a debugger and puzzle >> +* through it to remind myself... > > This repeats the commit message; and doesn't help me understanding the > {src/dst}_entry. (Maybe drop the first part here?) I'll read on. Yep, I'll toss it. >> +* >> +* If 'before' is renamed to 'after' then src_entry will contain >> +* the versions of 'before' from the merge_base, HEAD, and MERGE in >> +* stages 1, 2, and 3; dst_entry will contain the versions of >> +* 'after' from the merge_base, HEAD, and MERGE in stages 1, 2, and >> +* 3. > > So src == before, dst = after; no trickery with the stages (the same > stage number > before and after; only the order needs to be conveyed: > base, HEAD (ours?), MERGE (theirs?) > > I can understand that, so I wonder if we can phrase it to mention (base, > HEAD, MERGE) just once. Perhaps: If 'before' is renamed to 'after' then src_entry will contain the versions of 'before' from the merge_base, HEAD, and MERGE in stages 1, 2, and 3; and dst_entry will contain the respective versions of 'after' in corresponding locations. Thus, we have a total of six modes and oids, though some will be null. (Stage 0 is ignored; we're interested in handling conflicts.) ?
Re: [PATCH] t/lib-gpg: fix gpgconf stderr redirect to /dev/null
Hi Santiago, Santiago Torres wrote: Thanks for catching the redirection issue! I agree that the other fixes feel like overkill. Are you certain that switching to gpgconf --reload will have the same effect as --kill? (I know that this is the case for scdaemon only). I am not at all certain whether reload would work to fix the issues you fixed by killing the agent between runs. :) Were the ENOENT errors you encountered in running the tests multiple times easy to reproduce? If so, I can certainly try to reproduce them and then run the tests with --reload in place of --kill to gpgconf. If that worked across the various gnupg 2.x releases, it would be a simple enough change to make as a follow-up. -- Todd ~~ Whatever it is that the government does, sensible Americans would prefer that the government does it to somebody else. This is the idea behind foreign policy. -- P.J. O'Rourke signature.asc Description: PGP signature
Re: [PATCH v2 6/6] Testing: provide tests requiring them with ellipses after SHA-1 values
Where needed, we arrange for invocations of Git as if "-c core.printsha1ellipsis=true" had been specified on the command-line. This furnishes ellipses in the output which then matches what is expected. Signed-off-by: Ann T Ropea--- v2: rename patch series & focus on removal of ellipses t/t3040-subprojects-basic.sh | 12 t/t4013-diff-various.sh | 12 t/t9300-fast-import.sh | 12 3 files changed, 36 insertions(+) diff --git a/t/t3040-subprojects-basic.sh b/t/t3040-subprojects-basic.sh index 0a4ff6d824a0..63b85bfdd4f9 100755 --- a/t/t3040-subprojects-basic.sh +++ b/t/t3040-subprojects-basic.sh @@ -3,6 +3,18 @@ test_description='Basic subproject functionality' . ./test-lib.sh +# Some of the tests expect an ellipsis after the (abbreviated) +# SHA-1 value. The code below results in Git being called with +# "-c core.printsha1ellipsis=true" which satisfies those tests. +do_print_sha1_ellipsis="'core.printsha1ellipsis=true'" +if test -z "${GIT_CONFIG_PARAMETERS}" +then + GIT_CONFIG_PARAMETERS="${do_print_sha1_ellipsis}" +else + GIT_CONFIG_PARAMETERS="${GIT_CONFIG_PARAMETERS} ${do_print_sha1_ellipsis}" +fi +export GIT_CONFIG_PARAMETERS + test_expect_success 'setup: create superproject' ' : >Makefile && git add Makefile && diff --git a/t/t4013-diff-various.sh b/t/t4013-diff-various.sh index c515e3e53fee..8ee14c7c6796 100755 --- a/t/t4013-diff-various.sh +++ b/t/t4013-diff-various.sh @@ -7,6 +7,18 @@ test_description='Various diff formatting options' . ./test-lib.sh +# Some of the tests expect an ellipsis after the (abbreviated) +# SHA-1 value. The code below results in Git being called with +# "-c core.printsha1ellipsis=true" which satisfies those tests. +do_print_sha1_ellipsis="'core.printsha1ellipsis=true'" +if test -z "${GIT_CONFIG_PARAMETERS}" +then + GIT_CONFIG_PARAMETERS="${do_print_sha1_ellipsis}" +else + GIT_CONFIG_PARAMETERS="${GIT_CONFIG_PARAMETERS} ${do_print_sha1_ellipsis}" +fi +export GIT_CONFIG_PARAMETERS + LF=' ' diff --git a/t/t9300-fast-import.sh b/t/t9300-fast-import.sh index d47560b6343d..6cc41b90dafa 100755 --- a/t/t9300-fast-import.sh +++ b/t/t9300-fast-import.sh @@ -7,6 +7,18 @@ test_description='test git fast-import utility' . ./test-lib.sh . "$TEST_DIRECTORY"/diff-lib.sh ;# test-lib chdir's into trash +# Some of the tests expect an ellipsis after the (abbreviated) +# SHA-1 value. The code below results in Git being called with +# "-c core.printsha1ellipsis=true" which satisfies those tests. +do_print_sha1_ellipsis="'core.printsha1ellipsis=true'" +if test -z "${GIT_CONFIG_PARAMETERS}" +then + GIT_CONFIG_PARAMETERS="${do_print_sha1_ellipsis}" +else + GIT_CONFIG_PARAMETERS="${GIT_CONFIG_PARAMETERS} ${do_print_sha1_ellipsis}" +fi +export GIT_CONFIG_PARAMETERS + verify_packs () { for p in .git/objects/pack/*.pack do -- 2.13.6
Re: [PATCH v2 4/6] Documentation: user-manual: limit usage of ellipsis
Confusing the ellipsis with the three-dot operator should be made as difficult as possible. Signed-off-by: Ann T Ropea--- v2: rename patch series & focus on removal of ellipses Documentation/user-manual.txt | 22 +++--- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/Documentation/user-manual.txt b/Documentation/user-manual.txt index 3a03e63eb0d8..eff78902742a 100644 --- a/Documentation/user-manual.txt +++ b/Documentation/user-manual.txt @@ -319,7 +319,7 @@ do so (now or later) by using -b with the checkout command again. Example: git checkout -b new_branch_name -HEAD is now at 427abfa... Linux v2.6.17 +HEAD is now at 427abfa Linux v2.6.17 The HEAD then refers to the SHA-1 of the commit instead of to a branch, @@ -508,7 +508,7 @@ Bisecting: 3537 revisions left to test after this If you run `git branch` at this point, you'll see that Git has temporarily moved you in "(no branch)". HEAD is now detached from any -branch and points directly to a commit (with commit id 65934...) that +branch and points directly to a commit (with commit id 65934) that is reachable from "master" but not from v2.6.18. Compile and test it, and see whether it crashes. Assume it does crash. Then: @@ -549,14 +549,14 @@ says "bisect". Choose a safe-looking commit nearby, note its commit id, and check it out with: - -$ git reset --hard fb47ddb2db... +$ git reset --hard fb47ddb2db - then test, run `bisect good` or `bisect bad` as appropriate, and continue. Instead of `git bisect visualize` and then `git reset --hard -fb47ddb2db...`, you might just want to tell Git that you want to skip +fb47ddb2db`, you might just want to tell Git that you want to skip the current commit: - @@ -3416,7 +3416,7 @@ commit abc Author: Date: ... -:100644 100644 4b9458b... newsha... M somedirectory/myfile +:100644 100644 4b9458b newsha M somedirectory/myfile commit xyz @@ -3424,7 +3424,7 @@ Author: Date: ... -:100644 100644 oldsha... 4b9458b... M somedirectory/myfile +:100644 100644 oldsha 4b9458b M somedirectory/myfile This tells you that the immediately following version of the file was @@ -3449,7 +3449,7 @@ and your repository is good again! $ git log --raw --all -and just looked for the sha of the missing object (4b9458b..) in that +and just looked for the sha of the missing object (4b9458b) in that whole thing. It's up to you--Git does *have* a lot of information, it is just missing one particular blob version. @@ -4114,9 +4114,9 @@ program, e.g. `diff3`, `merge`, or Git's own merge-file, on the blob objects from these three stages yourself, like this: -$ git cat-file blob 263414f... >hello.c~1 -$ git cat-file blob 06fa6a2... >hello.c~2 -$ git cat-file blob cc44c73... >hello.c~3 +$ git cat-file blob 263414f >hello.c~1 +$ git cat-file blob 06fa6a2 >hello.c~2 +$ git cat-file blob cc44c73 >hello.c~3 $ git merge-file hello.c~2 hello.c~1 hello.c~3 @@ -4374,7 +4374,7 @@ $ git log --no-merges t/ In the pager (`less`), just search for "bundle", go a few lines back, -and see that it is in commit 18449ab0... Now just copy this object name, +and see that it is in commit 18449ab0. Now just copy this object name, and paste it into the command line --- -- 2.13.6
Re: [PATCH v2 3/6] diff: diff_aligned_abbrev: remove ellipsis after abbreviated SHA-1 value
Neither Git nor the user are in need of this (visual) aid anymore, but we must offer a transition period. Also, fix a typo: "abbbreviated" ---> "abbreviated". Signed-off-by: Ann T Ropea--- v2: rename patch series & focus on removal of ellipses diff.c | 69 +- 1 file changed, 39 insertions(+), 30 deletions(-) diff --git a/diff.c b/diff.c index 0763e89263ef..9709dc37c6d7 100644 --- a/diff.c +++ b/diff.c @@ -4902,41 +4902,50 @@ const char *diff_aligned_abbrev(const struct object_id *oid, int len) int abblen; const char *abbrev; + /* Do we want all 40 hex characters? +*/ if (len == GIT_SHA1_HEXSZ) return oid_to_hex(oid); - abbrev = diff_abbrev_oid(oid, len); - abblen = strlen(abbrev); - - /* -* In well-behaved cases, where the abbbreviated result is the -* same as the requested length, append three dots after the -* abbreviation (hence the whole logic is limited to the case -* where abblen < 37); when the actual abbreviated result is a -* bit longer than the requested length, we reduce the number -* of dots so that they match the well-behaved ones. However, -* if the actual abbreviation is longer than the requested -* length by more than three, we give up on aligning, and add -* three dots anyway, to indicate that the output is not the -* full object name. Yes, this may be suboptimal, but this -* appears only in "diff --raw --abbrev" output and it is not -* worth the effort to change it now. Note that this would -* likely to work fine when the automatic sizing of default -* abbreviation length is used--we would be fed -1 in "len" in -* that case, and will end up always appending three-dots, but -* the automatic sizing is supposed to give abblen that ensures -* uniqueness across all objects (statistically speaking). + /* An abbreviated value is fine, possibly followed by an +* ellipsis. */ - if (abblen < GIT_SHA1_HEXSZ - 3) { - static char hex[GIT_MAX_HEXSZ + 1]; - if (len < abblen && abblen <= len + 2) - xsnprintf(hex, sizeof(hex), "%s%.*s", abbrev, len+3-abblen, ".."); - else - xsnprintf(hex, sizeof(hex), "%s...", abbrev); - return hex; - } + if (print_sha1_ellipsis) { + abbrev = diff_abbrev_oid(oid, len); + abblen = strlen(abbrev); + + /* +* In well-behaved cases, where the abbreviated result is the +* same as the requested length, append three dots after the +* abbreviation (hence the whole logic is limited to the case +* where abblen < 37); when the actual abbreviated result is a +* bit longer than the requested length, we reduce the number +* of dots so that they match the well-behaved ones. However, +* if the actual abbreviation is longer than the requested +* length by more than three, we give up on aligning, and add +* three dots anyway, to indicate that the output is not the +* full object name. Yes, this may be suboptimal, but this +* appears only in "diff --raw --abbrev" output and it is not +* worth the effort to change it now. Note that this would +* likely to work fine when the automatic sizing of default +* abbreviation length is used--we would be fed -1 in "len" in +* that case, and will end up always appending three-dots, but +* the automatic sizing is supposed to give abblen that ensures +* uniqueness across all objects (statistically speaking). +*/ + if (abblen < GIT_SHA1_HEXSZ - 3) { + static char hex[GIT_MAX_HEXSZ + 1]; + if (len < abblen && abblen <= len + 2) + xsnprintf(hex, sizeof(hex), "%s%.*s", abbrev, len+3-abblen, ".."); + else + xsnprintf(hex, sizeof(hex), "%s...", abbrev); + return hex; + } - return oid_to_hex(oid); + return oid_to_hex(oid); + } else { + return diff_abbrev_oid(oid, len); + } } static void diff_flush_raw(struct diff_filepair *p, struct diff_options *opt) -- 2.13.6
Re: [PATCH v2 2/6] checkout: describe_detached_head: remove ellipsis after committish
We do not want an ellipsis displayed following an (abbreviated) SHA-1 value. The days when this was necessary to indicate the truncation to lower-level Git commands and/or the user are bygone. However, to ease the transition, the ellipsis will still be printed if the user (actively!) sets the config option core.printsha1ellipsis to true. Signed-off-by: Ann T Ropea--- v2: rename patch series & focus on removal of ellipses builtin/checkout.c | 9 +++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/builtin/checkout.c b/builtin/checkout.c index 6c2b4cd419a4..101a16a14a76 100644 --- a/builtin/checkout.c +++ b/builtin/checkout.c @@ -402,8 +402,13 @@ static void describe_detached_head(const char *msg, struct commit *commit) struct strbuf sb = STRBUF_INIT; if (!parse_commit(commit)) pp_commit_easy(CMIT_FMT_ONELINE, commit, ); - fprintf(stderr, "%s %s... %s\n", msg, - find_unique_abbrev(commit->object.oid.hash, DEFAULT_ABBREV), sb.buf); + if (print_sha1_ellipsis) { + fprintf(stderr, "%s %s... %s\n", msg, + find_unique_abbrev(commit->object.oid.hash, DEFAULT_ABBREV), sb.buf); + } else { + fprintf(stderr, "%s %s %s\n", msg, + find_unique_abbrev(commit->object.oid.hash, DEFAULT_ABBREV), sb.buf); + } strbuf_release(); } -- 2.13.6
Re: [PATCH v2 5/6] Documentation: revisions: fix typo: "three dot" ---> "three-dot" (in line with "two-dot").
Signed-off-by: Ann T Ropea--- v2: rename patch series & focus on removal of ellipses Documentation/revisions.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Documentation/revisions.txt b/Documentation/revisions.txt index 61277469c874..dfcc49c72c0f 100644 --- a/Documentation/revisions.txt +++ b/Documentation/revisions.txt @@ -271,7 +271,7 @@ The '..' (two-dot) Range Notation:: for commits that are reachable from r2 excluding those that are reachable from r1 by '{caret}r1 r2' and it can be written as 'r1..r2'. -The '...' (three dot) Symmetric Difference Notation:: +The '...' (three-dot) Symmetric Difference Notation:: A similar notation 'r1\...r2' is called symmetric difference of 'r1' and 'r2' and is defined as 'r1 r2 --not $(git merge-base --all r1 r2)'. -- 2.13.6
Re: [PATCH v2 1/6] config: introduce core.printsha1ellipsis
We use it to ascertain whether or not the user wants an ellipsis printed after an abbreviated SHA-1 value. By de-asserting it in the default, we encourage not printing the ellipsis in such circumstances. Not adding a corresponding cli option is intentional; and we would like to remove the config option *and* the ellipses after abbreviated SHA-1 values after a respectable period. Signed-off-by: Ann T Ropea--- v2: rename patch series & focus on removal of ellipses Documentation/config.txt | 10 ++ cache.h | 1 + config.c | 9 + environment.c| 1 + 4 files changed, 21 insertions(+) diff --git a/Documentation/config.txt b/Documentation/config.txt index 671fcbaa0fd1..93887820ff89 100644 --- a/Documentation/config.txt +++ b/Documentation/config.txt @@ -898,6 +898,16 @@ core.abbrev:: abbreviated object names to stay unique for some time. The minimum length is 4. +core.printsha1ellipsis (deprecated):: + A Boolean controlling whether an ellipsis should be + printed following an (abbreviated) SHA-1 value. This + affects indications of detached HEADs or the raw diff + output. Printing an ellipsis in the cases mentioned is no + longer considered adequate and support for it will be + removed in the foreseeable future (along with the + option). + Therefore, the default is false. + add.ignoreErrors:: add.ignore-errors (deprecated):: Tells 'git add' to continue adding files when some files cannot be diff --git a/cache.h b/cache.h index cb7fb7c004be..5cbc50a0c1ab 100644 --- a/cache.h +++ b/cache.h @@ -753,6 +753,7 @@ extern int check_stat; extern int quote_path_fully; extern int has_symlinks; extern int minimum_abbrev, default_abbrev; +extern int print_sha1_ellipsis; extern int ignore_case; extern int assume_unchanged; extern int prefer_symlink_refs; diff --git a/config.c b/config.c index 903abf9533b1..f560aea5e98b 100644 --- a/config.c +++ b/config.c @@ -1073,6 +1073,15 @@ static int git_default_core_config(const char *var, const char *value) return 0; } + /* Printing an ellipsis after an abbreviated SHA-1 value +* is no longer desired but must be selectable for some +* time to come. +*/ + if (!strcmp(var, "core.printsha1ellipsis")) { + print_sha1_ellipsis = git_config_bool(var, value); + return 0; + } + if (!strcmp(var, "core.disambiguate")) return set_disambiguate_hint_config(var, value); diff --git a/environment.c b/environment.c index 8289c25b44d7..5ceb92f921c6 100644 --- a/environment.c +++ b/environment.c @@ -19,6 +19,7 @@ int trust_ctime = 1; int check_stat = 1; int has_symlinks = 1; int minimum_abbrev = 4, default_abbrev = -1; +int print_sha1_ellipsis = 0; /* Only if the user requests it. */ int ignore_case; int assume_unchanged; int prefer_symlink_refs; -- 2.13.6
Re: [PATCH v2 2/4] progress: fix progress meters when dealing with lots of work
Thanks for the reviews! On Mon, Nov 13, 2017 at 1:33 PM, Jonathan Tanwrote: > On Mon, 13 Nov 2017 12:15:58 -0800 > Elijah Newren wrote: > >> -static int display(struct progress *progress, unsigned n, const char *done) >> +static int display(struct progress *progress, uint64_t n, const char *done) >> { >> const char *eol, *tp; >> >> @@ -106,7 +106,7 @@ static int display(struct progress *progress, unsigned >> n, const char *done) >> if (percent != progress->last_percent || progress_update) { >> progress->last_percent = percent; >> if (is_foreground_fd(fileno(stderr)) || done) { >> - fprintf(stderr, "%s: %3u%% (%u/%u)%s%s", >> + fprintf(stderr, "%s: %3u%% >> (%"PRIuMAX"/%"PRIuMAX")%s%s", >> progress->title, percent, n, >> progress->total, tp, eol); > > I think it would be better to cast the appropriate arguments to > uintmax_t - searching through the Git code shows that we do that in > several situations. Same for the rest of the diff. Interesting. My first inclination was to ask why not just change the variables to be of type uintmax_t instead of uint64_t (since we're already changing their types already), and then get rid of the cast. But I went digging through the source code based on your comment. Almost all the existing examples in the codebase were off_t and size_t values; there was only one case with uint64_t...but that one case led me to commit 5be507fc95 (Use PRIuMAX instead of 'unsigned long long' in show-index 2007-10-21), and that commit does suggest doing exactly as you say here. I'll fix it up.
Re: [PATCH] t/lib-gpg: fix gpgconf stderr redirect to /dev/null
> Of course, beyond getting stderr to /dev/null, there is the fact that on > versions of gnupg < 2.1, gpgconf --kill is not available. I noticed this with > gnupg-2.0.14 on CentOS 6. It also occurs on CentOS 7, which provides > gnupg-2.0.22. > > I don't know if there's much value in trying to better handle older gnupg-2.0 > systems. Hi Todd. Thanks for catching the redirection issue! I agree that the other fixes feel like overkill. Are you certain that switching to gpgconf --reload will have the same effect as --kill? (I know that this is the case for scdaemon only). Thanks again! -Santiago. signature.asc Description: PGP signature
Re: Bug in "revision.c: --all adds HEAD from all worktrees" ?
On Mon, Nov 13, 2017 at 2:03 PM, Luke Diamandwrote: > On 13 November 2017 at 19:51, Luke Diamand wrote: >> Hi! >> >> I think there may be a regression caused by this change which means >> that "git fetch origin" doesn't work: >> >> commit d0c39a49ccb5dfe7feba4325c3374d99ab123c59 (refs/bisect/bad) >> Author: Nguyễn Thái Ngọc Duy >> Date: Wed Aug 23 19:36:59 2017 +0700 >> >> revision.c: --all adds HEAD from all worktrees >> >> $ git fetch origin >> fatal: bad object HEAD >> error: ssh://my_remote_host/reponame did not send all necessary objects >> >> I used git bisect to find the problem, and it seems pretty consistent. >> "git fetch" with the previous revision works fine. >> >> FWIW, I've got a lot of git worktrees associated with this repo, so >> that may be why it's failing. The remote repo is actually a git-p4 >> clone, so HEAD there actually ends up pointing at >> refs/remote/p4/master. >> >> Thanks, >> Luke > > Quite a few of the worktrees have expired - their head revision has > been GC'd and no longer points to anything sensible > (gc.worktreePruneExpire). The function other_head_refs() in worktree.c > bails out if there's an error, which I think is the problem. I wonder > if it should instead just report something and then keep going. Also see https://public-inbox.org/git/cagz79kyp0z1g_h3nwfmshrawhmbocik5lepuxkj0nveebri...@mail.gmail.com/
Re: [PATCH 02/30] merge-recursive: Fix logic ordering issue
On Mon, Nov 13, 2017 at 2:04 PM, Elijah Newrenwrote: > Do you feel it's important that I come up with a demonstration case > here? If so, I'll see if I can generate one. I was mostly "just curious" on how you'd construct it theoretically. > it's actually something newly possible only due to directory rename detection. So something like {rename/delete} on a directory in the merge, but also an addition instead of the delete of another file? I wanted to debug a very similar issue today just after reviewing this series, see https://public-inbox.org/git/743acc29-85bb-3773-b6a0-68d4a0b8f...@ispras.ru/ Thanks, Stefan
INFO
I have important transaction for you as next of kin to claim US$8.37m email me at changgordo...@yahoo.com.hk so I can send you more details
Re: [PATCH 02/30] merge-recursive: Fix logic ordering issue
Thanks for the reviews! On Mon, Nov 13, 2017 at 11:48 AM, Stefan Bellerwrote: > On Fri, Nov 10, 2017 at 11:05 AM, Elijah Newren wrote: >> merge_trees() did a variety of work, including: >> * Calling get_unmerged() to get unmerged entries >> * Calling record_df_conflict_files() with all unmerged entries to >> do some work to ensure we could handle D/F conflicts correctly >> * Calling get_renames() to check for renames. >> >> An easily overlooked issue is that get_renames() can create more >> unmerged entries and add them to the list, which have the possibility of >> being involved in D/F conflicts. > > I presume these are created via insert_stage_data called in > get_renames, when the path entry is not found? Yes. >> So the call to >> record_df_conflict_files() should really be moved after all the rename >> detection. I didn't come up with any testcases demonstrating any bugs >> with the old ordering, but I suspect there were some for both normal >> renames and for directory renames. Fix the ordering. > > It is hard to trace this down, though looking at > 3af244caa8 (Cumulative update of merge-recursive in C, 2006-07-27) > may help us reason about it. It doesn't really go back that far. I added the record_df_conflict_files() function (originally named make_room_for_directories_of_df_conflicts()) in commit ef02b31721 (merge-recursive: Make room for directories in D/F conflicts 2010-09-20); the rename happened in commit 70cc3d36eb (merge-recursive: Save D/F conflict filenames instead of unlinking them 2011-08-11). > How would a bug look like? Some of these corner cases sometimes get confusing to try to reason about and duplicate, so I was trying to avoid thatoh, well. :-) I mostly wanted to use the simple logic that: record_df_conflict_files() exists to take an inventory of all unmerged files to make sure that D/F conflicts can be handled appropriately. get_renames() has the potential for adding more unmerged files, thus I should have placed record_df_conflict_files() after get_renames() when I introduced it. But since you asked... A bug here would essentially mean that a git merge fails to handle files in directories under a D/F conflict; when trying to process such files and write out their conflict state to disk, it would fail to create the necessary directory because a file is in the way. In order to trigger it, you'd need to have a D/F conflict where the file involved in the D/F conflict wasn't unmerged after unpack_trees() but only "shows up" due to the rename detection (i.e. added by the insert_stage_data() call as you mention above). I think reading through Documentation/technical/trivial-merge.txt, that this actually isn't possible with what I'm calling "normal" renames; it's actually something newly possible only due to directory rename detection. But you may have to get the merge direction just right, you might have to worry about files that sort between a file with the same name as a directory and the files within the directory (e.g. "path.txt" in the list "path", then "path.txt", then "path/foo"). Do you feel it's important that I come up with a demonstration case here? If so, I'll see if I can generate one.
Re: [PATCH 04/30] directory rename detection: basic testcases
On Fri, Nov 10, 2017 at 11:05 AM, Elijah Newrenwrote: > Signed-off-by: Elijah Newren > --- > t/t6043-merge-rename-directories.sh | 391 > > 1 file changed, 391 insertions(+) > create mode 100755 t/t6043-merge-rename-directories.sh > > diff --git a/t/t6043-merge-rename-directories.sh > b/t/t6043-merge-rename-directories.sh > new file mode 100755 > index 00..b737b0a105 > --- /dev/null > +++ b/t/t6043-merge-rename-directories.sh > @@ -0,0 +1,391 @@ > +#!/bin/sh > + > +test_description="recursive merge with directory renames" > +# includes checking of many corner cases, with a similar methodology to: > +# t6042: corner cases with renames but not criss-cross merges > +# t6036: corner cases with both renames and criss-cross merges > +# > +# The setup for all of them, pictorially, is: > +# > +# B > +# o > +# / \ > +# A o ? > +# \ / > +# o > +# C > +# > +# To help make it easier to follow the flow of tests, they have been > +# divided into sections and each test will start with a quick explanation > +# of what commits A, B, and C contain. > +# > +# Notation: > +#z/{b,c} means files z/b and z/c both exist > +#x/d_1 means file x/d exists with content d1. (Purpose of the > +# underscore notation is to differentiate different > +# files that might be renamed into each other's paths.) > + > +. ./test-lib.sh > + > + > +### > +# SECTION 1: Basic cases we should be able to handle > +### > + > +# Testcase 1a, Basic directory rename. > +# Commit A: z/{b,c} > +# Commit B: y/{b,c} > +# Commit C: z/{b,c,d,e/f} (minor thought:) After rereading the docs above this is clear; I wonder if instead of A, B, C a notation of Base, ours, theirs would be easier to understand? > +test_expect_success '1a-setup: Simple directory rename detection' ' > +test_expect_failure '1a-check: Simple directory rename detection' ' Thanks for splitting the setup and the check into two different test cases! > + git checkout B^0 && Any reason for ^0 ? (to make clear it is a branch?) > +# Testcase 1b, Merge a directory with another > +# Commit A: z/{b,c}, y/d > +# Commit B: z/{b,c,e}, y/d > +# Commit C: y/{b,c,d} > +# Expected: y/{b,c,d,e} > + > +test_expect_success '1b-setup: Merge a directory with another' ' > + git rm -rf . && > + git clean -fdqx && > + rm -rf .git && > + git init && This is quite a strong statement to start a test with. Nowadays we rather do test_when_finished "some command" && than polluting the follow up tests. But as you split up the previous test into 2 tests, it is not clear if this would bring any good. Also these are four cleanup commands, I'd have expected fewer. Maybe just "rm -rf ." ? Or as we make a new repo for each test case, test_create_repo 1a && cd 1a in the first setup, and here we do test_create_repo 1b && cd 1b relying on test_done to cleanup everything afterwards? > +# Testcase 1c, Transitive renaming > +# (Related to testcases 3a and 6d -- when should a transitive rename > apply?) > +# (Related to testcases 9c and 9d -- can transitivity repeat?) > +# Commit A: z/{b,c}, x/d > +# Commit B: y/{b,c}, x/d > +# Commit C: z/{b,c,d} So B is "Rename z to y" and C is "Move x/d to z/d" > +# Expected: y/{b,c,d} (because x/d -> z/d -> y/d) This is an excellent expectation for a clean case like this. I have not reached 3, 9 yet, so I'll remember these questions. > +test_expect_success '1c-setup: Transitive renaming' ' > + git rm -rf . && > + git clean -fdqx && > + rm -rf .git && > + git init && > + > + mkdir z && > + echo b >z/b && > + echo c >z/c && > + mkdir x && > + echo d >x/d && > + git add z x && > + test_tick && > + git commit -m "A" && > + > + git branch A && > + git branch B && > + git branch C && > + > + git checkout B && > + git mv z y && > + test_tick && > + git commit -m "B" && > + > + git checkout C && > + git mv x/d z/d && > + test_tick && > + git commit -m "C" > +' > + > +test_expect_failure '1c-check: Transitive renaming' ' > + git checkout B^0 && > + > + git merge -s recursive C^0 && > + > + test 3 -eq $(git ls-files -s | wc -l) && git ls-files >out && test_line_count = 3 out && maybe? Piping output of git commands somewhere is an anti-pattern as we cannot examine the return code of ls-files in this case. > + test $(git rev-parse HEAD:y/b) = $(git rev-parse A:z/b) && > + test $(git rev-parse HEAD:y/c) = $(git rev-parse A:z/c) && > + test $(git rev-parse HEAD:y/d) = $(git rev-parse A:x/d) && Speaking of
Re: Bug in "revision.c: --all adds HEAD from all worktrees" ?
On 13 November 2017 at 19:51, Luke Diamandwrote: > Hi! > > I think there may be a regression caused by this change which means > that "git fetch origin" doesn't work: > > commit d0c39a49ccb5dfe7feba4325c3374d99ab123c59 (refs/bisect/bad) > Author: Nguyễn Thái Ngọc Duy > Date: Wed Aug 23 19:36:59 2017 +0700 > > revision.c: --all adds HEAD from all worktrees > > $ git fetch origin > fatal: bad object HEAD > error: ssh://my_remote_host/reponame did not send all necessary objects > > I used git bisect to find the problem, and it seems pretty consistent. > "git fetch" with the previous revision works fine. > > FWIW, I've got a lot of git worktrees associated with this repo, so > that may be why it's failing. The remote repo is actually a git-p4 > clone, so HEAD there actually ends up pointing at > refs/remote/p4/master. > > Thanks, > Luke Quite a few of the worktrees have expired - their head revision has been GC'd and no longer points to anything sensible (gc.worktreePruneExpire). The function other_head_refs() in worktree.c bails out if there's an error, which I think is the problem. I wonder if it should instead just report something and then keep going.
[buglet] gitk and git cherry-pick --abort interaction
Apologies in advance for the vagueness of this bug report. I was juggling a few patches around in a git repo (happens to be linux but that's probably not particularly relevant). I'd been reverting, rebasing and cherry-picking on the CLI. Then I found I needed one more commit which I located with gitk. Since it was already open I used the cherry-pick option within gitk, there were conflicts and gitk invoked git citool for me. At that point I decided to bail on the cherry-pick and closed citool and gitk and ran git cherry-pick --abort from the command line and that’s where things got weird. The abort put me on a different commit than where I started (which happened to be the commit from a previous am --abort). I'm guessing gitk's cherry-pick doesn't fully record the information in a way that the cli counterpart expects. I'll try and get a reproduction going but in the meantime some info from my terminal history and reflog might help. $ git --version git version 2.15.0 $ cd linux/ linux (edac u+10)$ gitk # not pictured here cherry-picking one commit in gitk and citool running linux (edac *+|CHERRY-PICKING u+10)$ linux (edac *+|CHERRY-PICKING u+10)$ git cherry-pick --abort linux (edac u+13)$ Here's some info from the reflog abridged so gmail doesn't eat it. linux (edac u+13)$ git reflog d2b10042ccaf (HEAD -> edac) HEAD@{0}: reset: moving to d2b10042ccaf 67ebefac4b7e HEAD@{1}: rebase -i (finish): returning to refs/heads/edac ... HEAD@{2} through HEAD@{33} are rebase/commit/am/cherry-pick d2b10042ccaf (HEAD -> edac) HEAD@{34}: am --abort Thanks, Chris
no mountable file systems
Dear team, when i download Git on Mac it says “no mountable file systems” and doesnt open after. how can i solve this? Thank you for your help,
Re: [PATCH v2 2/4] progress: fix progress meters when dealing with lots of work
On Mon, 13 Nov 2017 12:15:58 -0800 Elijah Newrenwrote: > -static int display(struct progress *progress, unsigned n, const char *done) > +static int display(struct progress *progress, uint64_t n, const char *done) > { > const char *eol, *tp; > > @@ -106,7 +106,7 @@ static int display(struct progress *progress, unsigned n, > const char *done) > if (percent != progress->last_percent || progress_update) { > progress->last_percent = percent; > if (is_foreground_fd(fileno(stderr)) || done) { > - fprintf(stderr, "%s: %3u%% (%u/%u)%s%s", > + fprintf(stderr, "%s: %3u%% > (%"PRIuMAX"/%"PRIuMAX")%s%s", > progress->title, percent, n, > progress->total, tp, eol); I think it would be better to cast the appropriate arguments to uintmax_t - searching through the Git code shows that we do that in several situations. Same for the rest of the diff.
Recovering from gc errors
(I'm using git 2.15.0.) So today "git gc" started complaining: error: Could not read 2bc277bcb7e9cc6ef2ea677dd1c3dcd1f9af0c2b fatal: Failed to traverse parents of commit 9c355a7726e31b3033b8e714cf7edb4f0a41d8d4 error: failed to run repack I suspect I'm a victim of the worktree+submodule bugs -- as a longtime user of contrib/workdir/git-new-workdir, I've been playing with the "worktree" command since it was first introduced. The "git gc" error occurs when it's run in my main repo; I have not tried it in any of my worktrees/workdirs. Various incantations of "git show ... 9c355a7726e31" only fail with the same error, so I can't determine much about the problematic commit. Luckily I'm not particularly concerned with losing objects, as I push any important progress to named refs in backup repos. But I would like to clean this up in my local repo so that gc stops failing. I tried simply removing this and other loose commits that trip up gc (i.e. the objects/9c/355a7726e31b3033b8e714cf7edb4f0a41d8d4 file -- there are 49 such files, all of which are several months old), but now gc complains of a bad tree object: error: Could not read c1a99c3520f0b456b8025c50302a4cc9b0b2d777 fatal: bad tree object c1a99c3520f0b456b8025c50302a4cc9b0b2d777 error: failed to run repack This object is not lying around loose. "git fsck" lists several dangling blob/commit/tree objects, but none of them are c1a99c3520f0b4. So I'm not sure what to do next. Any suggestions? Thanks, M.
[PATCH] rebase: fix stderr redirect in apply_autostash()
The intention is to ignore all output from the 'git stash apply' call. Adjust the order of the redirection to ensure that both stdout and stderr are redirected to /dev/null. Signed-off-by: Todd Zullinger--- git-rebase.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/git-rebase.sh b/git-rebase.sh index 6344e8d5e3..aabbf6b69e 100755 --- a/git-rebase.sh +++ b/git-rebase.sh @@ -166,7 +166,7 @@ apply_autostash () { if test -f "$state_dir/autostash" then stash_sha1=$(cat "$state_dir/autostash") - if git stash apply $stash_sha1 2>&1 >/dev/null + if git stash apply $stash_sha1 >/dev/null 2>&1 then echo "$(gettext 'Applied autostash.')" >&2 else -- 2.15.0
Re: [PATCH 1/2] merge: close the index lock when not writing the new index
On 11 November 2017 at 00:13, Joel Teichroebwrote: > If the merge does not have anything to do, it does not unlock the index, > causing any further index operations to fail. Thus, always unlock the index > regardless of outcome. > if (clean < 0) > return clean; Do we need to roll back the lock also if `clean` is negative? The current callers are built-ins which will error out, but future callers might be caught off guard by this. > - if (active_cache_changed && > - write_locked_index(_index, , COMMIT_LOCK)) > - return err(o, _("Unable to write index.")); > + if (active_cache_changed) { > + if (write_locked_index(_index, , COMMIT_LOCK)) > + return err(o, _("Unable to write index.")); > + } else { > + rollback_lock_file(); > + } > > return clean ? 0 : 1; > } Looks correct. A simpler change which would still match the commit message would be to unconditionally call `rollback_lock_file()` just before returning. That would perhaps be slightly more future-proof, since it will always leave the lock unlocked, even if the if-else grows more complicated. Well, "always" modulo returning early and forgetting to roll back the lock. ;-) Looking at existing code, it's not obvious which way we should prefer. Just a thought. Thanks for spotting this. I was poking around here recently, but failed to notice this lax lock-handling. Martin
[PATCH] t/lib-gpg: fix gpgconf stderr redirect to /dev/null
In 29ff1f8f74 (t: lib-gpg: flush gpg agent on startup, 2017-07-20), a call to gpgconf was added to kill the gpg-agent. The intention was to ignore all output from the call, but the order of the redirection needs to be switched to ensure that both stdout and stderr are redirected to /dev/null. Without this, gpgconf from gnupg-2.0 releases would output 'gpgconf: invalid option "--kill"' each time it was called. Signed-off-by: Todd Zullinger--- I noticed that gpgconf produced error output for a number of tests on CentOS/RHEL. As an example: *** t5534-push-signed.sh *** gpgconf: invalid option "--kill" Looking at the code in lib-gpg.sh, it appeared the intention was to ignore this output. Reading through the review of the patch confirmed that feeling[1]. The current code gets caught by the subtleties of output redirection. (Who hasn't been burned at some point by the difference between '2>&1 >/dev/null' and '>/dev/null 2>&1' ? ;) [1] https://public-inbox.org/git/xmqq379qlvzi@gitster.mtv.corp.google.com/ Of course, beyond getting stderr to /dev/null, there is the fact that on versions of gnupg < 2.1, gpgconf --kill is not available. I noticed this with gnupg-2.0.14 on CentOS 6. It also occurs on CentOS 7, which provides gnupg-2.0.22. I don't know if there's much value in trying to better handle older gnupg-2.0 systems. Using gpgconf --reload might be sufficient to work on gnupg-2.0 and newer systems. That might solve the issues with gpg-agent caching stale file handles that motivated the initial patch. If not, finding what works well with both gnupg-2.0 and newer seems mildly painful. This method works for me on 2.0, 2.1, and 2.2: pid=$(echo GETINFO pid | gpg-connect-agent | awk '/^D / {print $2}') And people say crypto tools aren't intuitive. Pfff. :/ A fairly gross way to use that in lib-gpg.sh might be: diff --git c/t/lib-gpg.sh w/t/lib-gpg.sh index 43679a4c64..c91d9b334f 100755 --- c/t/lib-gpg.sh +++ w/t/lib-gpg.sh @@ -1,5 +1,10 @@ #!/bin/sh +gpg_killagent() { + pid=$(echo GETINFO pid | gpg-connect-agent | awk '/^D / {print $2}') + test -n "$pid" && kill "$pid" +} + gpg_version=$(gpg --version 2>&1) if test $? != 127 then @@ -31,7 +36,7 @@ then chmod 0700 ./gpghome && GNUPGHOME="$(pwd)/gpghome" && export GNUPGHOME && - (gpgconf --kill gpg-agent 2>&1 >/dev/null || : ) && + (gpg_killagent >/dev/null 2>&1 || : ) && gpg --homedir "${GNUPGHOME}" 2>/dev/null --import \ "$TEST_DIRECTORY"/lib-gpg/keyring.gpg && gpg --homedir "${GNUPGHOME}" 2>/dev/null --import-ownertrust \ I have my doubts that doing something like the above is worthwhile. It's probably good enough to simply fix up the gpgconf stderr redirection and call it a day. I haven't seen any gpg failures in the test runs I've done, so I haven't had a need to re-run those tests. (I also have not run the test suite with the ugly gpg_killagent() diff above. I have run it with the change to fix stderr redirection and confirmed it succeeds without the gpgconf error messages.) Lastly, I also noticed that git-rebase.sh uses the same 2>&1 >/dev/null. I suspect it's similarly not intentional: $ git grep -h -C4 '2>&1 >/dev/null' -- git-rebase.sh apply_autostash () { if test -f "$state_dir/autostash" then stash_sha1=$(cat "$state_dir/autostash") if git stash apply $stash_sha1 2>&1 >/dev/null then echo "$(gettext 'Applied autostash.')" >&2 else git stash store -m "autostash" -q $stash_sha1 || I'll send a separate patch to adjust that code as well. t/lib-gpg.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/t/lib-gpg.sh b/t/lib-gpg.sh index 43679a4c64..a5d3b2cbaa 100755 --- a/t/lib-gpg.sh +++ b/t/lib-gpg.sh @@ -31,7 +31,7 @@ then chmod 0700 ./gpghome && GNUPGHOME="$(pwd)/gpghome" && export GNUPGHOME && - (gpgconf --kill gpg-agent 2>&1 >/dev/null || : ) && + (gpgconf --kill gpg-agent >/dev/null 2>&1 || : ) && gpg --homedir "${GNUPGHOME}" 2>/dev/null --import \ "$TEST_DIRECTORY"/lib-gpg/keyring.gpg && gpg --homedir "${GNUPGHOME}" 2>/dev/null --import-ownertrust \ -- 2.15.0
Re: man page for "git-worktree" is a bit confusing WRT "prune"
On Mon, 13 Nov 2017, Eric Sunshine wrote: > On Mon, Nov 13, 2017 at 9:48 AM, Robert P. J. Day> wrote: ... snip ... > > finally, the prune "--expire" option is truly confusing: > > > > --expire > > With prune, only expire unused working trees older than . > > > > suddenly, we encounter the verb "expire", which means ... what? > > how does "expiring" a worktree differ from "pruning" a worktree? > > and what makes a worktree "unused"? the normal meaning of "unused" > > is that you haven't, you know, *used* it lately. in this context, > > though, does it mean deleted? and if it means deleted, what does > > it mean for it to be older than some time if it's already gone? > > This dates back to the original behavior of automatically pruning > administrative information for deleted worktrees. As discussed > elsewhere in the document, a worktree may be placed on some > removable device (USB drive, memory stick, etc.) or network share > which isn't always mounted. The "expire time" provides such > not-necessarily-mounted worktrees a grace period before being pruned > automatically. how is this "expire time" measured? relative to what? i've looked under .git/worktrees/, and i see a bunch of files defining that worktree, but it's not clear how a worktree stores the relevant time to be used for the determination of when a worktree "expires". oh, and is it fair to assume that if a worktree is temporaily missing, and is subsequently restored, the expire time counter is reset? otherwise, it would get kind of weird. rday -- Robert P. J. Day Ottawa, Ontario, CANADA http://crashcourse.ca Twitter: http://twitter.com/rpjday LinkedIn: http://ca.linkedin.com/in/rpjday
Re: [PATCH 03/30] merge-recursive: Add explanation for src_entry and dst_entry
On Fri, Nov 10, 2017 at 11:05 AM, Elijah Newrenwrote: > If I have to walk through the debugger and inspect the values found in > here in order to figure out their meaning, despite having known these > things inside and out some years back, then they probably need a comment > for the casual reader to explain their purpose. > > Signed-off-by: Elijah Newren > --- > merge-recursive.c | 22 ++ > 1 file changed, 22 insertions(+) > > diff --git a/merge-recursive.c b/merge-recursive.c > index 52521faf09..3526c8d0b8 100644 > --- a/merge-recursive.c > +++ b/merge-recursive.c > @@ -513,6 +513,28 @@ static void record_df_conflict_files(struct > merge_options *o, > > struct rename { > struct diff_filepair *pair; > + /* > +* Because I keep forgetting every few years what src_entry and > +* dst_entry are and have to walk through a debugger and puzzle > +* through it to remind myself... This repeats the commit message; and doesn't help me understanding the {src/dst}_entry. (Maybe drop the first part here?) I'll read on. > +* > +* If 'before' is renamed to 'after' then src_entry will contain > +* the versions of 'before' from the merge_base, HEAD, and MERGE in > +* stages 1, 2, and 3; dst_entry will contain the versions of > +* 'after' from the merge_base, HEAD, and MERGE in stages 1, 2, and > +* 3. So src == before, dst = after; no trickery with the stages (the same stage number before and after; only the order needs to be conveyed: base, HEAD (ours?), MERGE (theirs?) I can understand that, so I wonder if we can phrase it to mention (base, HEAD, MERGE) just once. > Thus, we have a total of six modes and oids, though some > +* will be null. (Stage 0 is ignored; we're interested in handling s/will be/may be/ or /can be/? > +* conflicts.) > +* > +* Since we don't turn on break-rewrites by default, neither > +* src_entry nor dst_entry can have all three of their stages have > +* non-null oids, meaning at most four of the six will be non-null. Oh. That explains the choice of /will be/ above. Thanks! > +* Also, since this is a rename, both src_entry and dst_entry will > +* have at least one non-null oid, meaning at least two will be > +* non-null. Of the six oids, a typical rename will have three be > +* non-null. Only two implies a rename/delete, and four implies a > +* rename/add. That makes sense. Thanks, Stefan > +*/ > struct stage_data *src_entry; > struct stage_data *dst_entry; > unsigned processed:1; > -- > 2.15.0.5.g9567be9905 >
Re: [PATCH v2 2/4] progress: fix progress meters when dealing with lots of work
On Mon, 13 Nov 2017 12:15:58 -0800 Elijah Newrenwrote: > The possibility of setting merge.renameLimit beyond 2^16 raises the > possibility that the values passed to progress can exceed 2^32. > Use uint64_t, because it "ought to be enough for anybody". :-) > > Signed-off-by: Elijah Newren > --- > This does imply 64-bit math for all progress operations. Possible > alternatives > I could think of listed at > https://public-inbox.org/git/CABPp-BH1Cpc9UfYpmBBAHWSqadg=QuD=28qx1ov29zdvf4n...@mail.gmail.com/ > Opinions of others on whether 64-bit is okay, or preference for which > alternative > is picked? I haven't looked into this in much detail, but another alternative to consider is to use size_t everywhere. This also allows us to use st_add and st_mult, which checks overflow for us. Changing progress to use the size_t of the local machine makes sense to me.
[PATCH v2 4/4] sequencer: show rename progress during cherry picks
When trying to cherry-pick a change that has lots of renames, it is somewhat unsettling to wait a really long time without any feedback. Signed-off-by: Elijah Newren--- sequencer.c | 1 + 1 file changed, 1 insertion(+) diff --git a/sequencer.c b/sequencer.c index 2b4cecb617..247d93f363 100644 --- a/sequencer.c +++ b/sequencer.c @@ -448,6 +448,7 @@ static int do_recursive_merge(struct commit *base, struct commit *next, o.branch2 = next ? next_label : "(empty tree)"; if (is_rebase_i(opts)) o.buffer_output = 2; + o.show_rename_progress = 1; head_tree = parse_tree_indirect(head); next_tree = next ? next->tree : empty_tree(); -- 2.15.0.44.gf995e411c7
[PATCH v2 3/4] Remove silent clamp of renameLimit
In commit 0024a5492 (Fix the rename detection limit checking; 2007-09-14), the renameLimit was clamped to 32767. This appears to have been to simply avoid integer overflow in the following computation: num_create * num_src <= rename_limit * rename_limit although it also could be viewed as a hardcoded bound on the amount of CPU time we're willing to allow users to tell git to spend on handling renames. An upper bound may make sense, but unfortunately this upper bound was neither communicated to the users, nor documented anywhere. Although large limits can make things slow, we have users who would be ecstatic to have a small five file change be correctly cherry picked even if they have to manually specify a large limit and wait ten minutes for the renames to be detected. Signed-off-by: Elijah Newren--- diff.c| 2 +- diffcore-rename.c | 11 --- 2 files changed, 5 insertions(+), 8 deletions(-) diff --git a/diff.c b/diff.c index 6fd288420b..c6597e3231 100644 --- a/diff.c +++ b/diff.c @@ -5524,7 +5524,7 @@ void diff_warn_rename_limit(const char *varname, int needed, int degraded_cc) warning(_(rename_limit_warning)); else return; - if (0 < needed && needed < 32767) + if (0 < needed) warning(_(rename_limit_advice), varname, needed); } diff --git a/diffcore-rename.c b/diffcore-rename.c index c03f484d53..32366632f4 100644 --- a/diffcore-rename.c +++ b/diffcore-rename.c @@ -391,14 +391,10 @@ static int too_many_rename_candidates(int num_create, * growing larger than a "rename_limit" square matrix, ie: * *num_create * num_src > rename_limit * rename_limit -* -* but handles the potential overflow case specially (and we -* assume at least 32-bit integers) */ - if (rename_limit <= 0 || rename_limit > 32767) - rename_limit = 32767; if ((num_create <= rename_limit || num_src <= rename_limit) && - (num_create * num_src <= rename_limit * rename_limit)) + ((uint64_t)num_create * (uint64_t)num_src +<= (uint64_t)rename_limit * (uint64_t)rename_limit)) return 0; options->needed_rename_limit = @@ -415,7 +411,8 @@ static int too_many_rename_candidates(int num_create, num_src++; } if ((num_create <= rename_limit || num_src <= rename_limit) && - (num_create * num_src <= rename_limit * rename_limit)) + ((uint64_t)num_create * (uint64_t)num_src +<= (uint64_t)rename_limit * (uint64_t)rename_limit)) return 2; return 1; } -- 2.15.0.44.gf995e411c7
[PATCH v2 0/4] Fix issues with rename detection limits
This patchset fixes some issues around rename detection limits. Changes since the original submission[1]: * Patches 2 and 3 are swapped in order so as to not temporarily introduce any bugs (even if only cosmetic) * Commit message fixups * Using uint64_t instead of double for holding multiplication of integers * Corrected format specifier for printing 64-bit ints. [1] https://public-inbox.org/git/20171110173956.25105-1-new...@gmail.com/ Elijah Newren (4): sequencer: warn when internal merge may be suboptimal due to renameLimit progress: fix progress meters when dealing with lots of work Remove silent clamp of renameLimit sequencer: show rename progress during cherry picks diff.c| 2 +- diffcore-rename.c | 15 ++- progress.c| 22 +++--- progress.h| 8 sequencer.c | 2 ++ 5 files changed, 24 insertions(+), 25 deletions(-) -- 2.15.0.44.gf995e411c7
[PATCH v2 2/4] progress: fix progress meters when dealing with lots of work
The possibility of setting merge.renameLimit beyond 2^16 raises the possibility that the values passed to progress can exceed 2^32. Use uint64_t, because it "ought to be enough for anybody". :-) Signed-off-by: Elijah Newren--- This does imply 64-bit math for all progress operations. Possible alternatives I could think of listed at https://public-inbox.org/git/CABPp-BH1Cpc9UfYpmBBAHWSqadg=QuD=28qx1ov29zdvf4n...@mail.gmail.com/ Opinions of others on whether 64-bit is okay, or preference for which alternative is picked? diffcore-rename.c | 4 ++-- progress.c| 22 +++--- progress.h| 8 3 files changed, 17 insertions(+), 17 deletions(-) diff --git a/diffcore-rename.c b/diffcore-rename.c index 0d8c3d2ee4..c03f484d53 100644 --- a/diffcore-rename.c +++ b/diffcore-rename.c @@ -534,7 +534,7 @@ void diffcore_rename(struct diff_options *options) if (options->show_rename_progress) { progress = start_delayed_progress( _("Performing inexact rename detection"), - rename_dst_nr * rename_src_nr); + (uint64_t)rename_dst_nr * (uint64_t)rename_src_nr); } mx = xcalloc(st_mult(NUM_CANDIDATE_PER_DST, num_create), sizeof(*mx)); @@ -571,7 +571,7 @@ void diffcore_rename(struct diff_options *options) diff_free_filespec_blob(two); } dst_cnt++; - display_progress(progress, (i+1)*rename_src_nr); + display_progress(progress, (uint64_t)(i+1)*(uint64_t)rename_src_nr); } stop_progress(); diff --git a/progress.c b/progress.c index 289678d43d..837d3a8eb8 100644 --- a/progress.c +++ b/progress.c @@ -30,8 +30,8 @@ struct throughput { struct progress { const char *title; - int last_value; - unsigned total; + uint64_t last_value; + uint64_t total; unsigned last_percent; unsigned delay; unsigned delayed_percent_threshold; @@ -79,7 +79,7 @@ static int is_foreground_fd(int fd) return tpgrp < 0 || tpgrp == getpgid(0); } -static int display(struct progress *progress, unsigned n, const char *done) +static int display(struct progress *progress, uint64_t n, const char *done) { const char *eol, *tp; @@ -106,7 +106,7 @@ static int display(struct progress *progress, unsigned n, const char *done) if (percent != progress->last_percent || progress_update) { progress->last_percent = percent; if (is_foreground_fd(fileno(stderr)) || done) { - fprintf(stderr, "%s: %3u%% (%u/%u)%s%s", + fprintf(stderr, "%s: %3u%% (%"PRIuMAX"/%"PRIuMAX")%s%s", progress->title, percent, n, progress->total, tp, eol); fflush(stderr); @@ -116,7 +116,7 @@ static int display(struct progress *progress, unsigned n, const char *done) } } else if (progress_update) { if (is_foreground_fd(fileno(stderr)) || done) { - fprintf(stderr, "%s: %u%s%s", + fprintf(stderr, "%s: %"PRIuMAX"%s%s", progress->title, n, tp, eol); fflush(stderr); } @@ -127,7 +127,7 @@ static int display(struct progress *progress, unsigned n, const char *done) return 0; } -static void throughput_string(struct strbuf *buf, off_t total, +static void throughput_string(struct strbuf *buf, uint64_t total, unsigned int rate) { strbuf_reset(buf); @@ -138,7 +138,7 @@ static void throughput_string(struct strbuf *buf, off_t total, strbuf_addstr(buf, "/s"); } -void display_throughput(struct progress *progress, off_t total) +void display_throughput(struct progress *progress, uint64_t total) { struct throughput *tp; uint64_t now_ns; @@ -200,12 +200,12 @@ void display_throughput(struct progress *progress, off_t total) display(progress, progress->last_value, NULL); } -int display_progress(struct progress *progress, unsigned n) +int display_progress(struct progress *progress, uint64_t n) { return progress ? display(progress, n, NULL) : 0; } -static struct progress *start_progress_delay(const char *title, unsigned total, +static struct progress *start_progress_delay(const char *title, uint64_t total, unsigned percent_threshold, unsigned delay) { struct progress *progress = malloc(sizeof(*progress)); @@ -227,12 +227,12 @@ static struct progress *start_progress_delay(const char *title, unsigned total, return progress; } -struct progress *start_delayed_progress(const char *title, unsigned total)
[PATCH v2 1/4] sequencer: warn when internal merge may be suboptimal due to renameLimit
When many files were renamed, the recursive merge strategy stopped detecting renames and left many paths with delete/modify conflicts, without any warning about what was going on or providing any hints about how to tell Git to spend more cycles to detect renames. Signed-off-by: Elijah Newren--- sequencer.c | 1 + 1 file changed, 1 insertion(+) diff --git a/sequencer.c b/sequencer.c index f2a10cc4f2..2b4cecb617 100644 --- a/sequencer.c +++ b/sequencer.c @@ -462,6 +462,7 @@ static int do_recursive_merge(struct commit *base, struct commit *next, if (is_rebase_i(opts) && clean <= 0) fputs(o.obuf.buf, stdout); strbuf_release(); + diff_warn_rename_limit("merge.renamelimit", o.needed_rename_limit, 0); if (clean < 0) return clean; -- 2.15.0.44.gf995e411c7
Re: [PATCH 3/4] progress: Fix progress meters when dealing with lots of work
Thanks for the reviews and suggestions! On Sun, Nov 12, 2017 at 9:24 PM, Junio C Hamanowrote: > Elijah Newren writes: > >> Subject: Re: [PATCH 3/4] progress: Fix progress meters when dealing with >> lots of work > > Style: s/Fix/fix/; I also messed this up in a lot of my patches in my other patch series. I've fixed them all up, but I'll wait to resubmit those other series until I get some other reviews. > The middle part of the log message may waste more mental bandwidth > of readers than it is worth. It might have gave you satisfaction to > be able to vent, but don't (the place to do so is after the three > dash lines). Cleaned it up, along with the other commit message you pointed out; I'll resubmit shortly. > I am not sure if we want all codepaths to do 64-bit math for > progress meter, but let's see what others would think. If others don't want to do 64-bit math for the progress meter, what would they like to see done instead? I can see a few options: 1) Have two separate progress codepaths, one for 32-bith math and one for 64-bit math. 2) Instead of counting pairs of source/dest files compared, just count number of dest paths completed. (Thus, we wouldn't need a value big enough to hold rename_dst_nr * rename_src_nr, just big enough to hold rename_dst_nr). 3) just let the progress meter overflow and show nonsensical values 4) don't show the progress meter if overflow would happen 5) something else I'm not thinking of. >> - fprintf(stderr, "%s: %3u%% (%u/%u)%s%s", >> + fprintf(stderr, "%s: %3u%% (%lu/%lu)%s%s", > > Are these (and there are probably other instances in this patch) %lu > correct? Oops, no. I think %llu is right, though looking around the code it appears folks use PRIuMAX and avoid %llu due to possible issues with old windows compilers. Not sure if that's still relevant, but I'll try to remain consistent with what I see elsewhere and include that fix in my re-roll.
Re: [RFC 0/3] Add support for --cover-at-tip
Le 13/11/2017 à 20:40, Jonathan Tan a écrit : > On Mon, 13 Nov 2017 18:13:27 +0100 > Nicolas Morey-Chaisemartinwrote: > >> v2: >> - Enhance mailinfo to parse patch series id from subject >> - Detect cover using mailinfo parsed ids in git am > I noticed that this was done in the patch set by searching for "PATCH" - > that is probably quite error-prone as not all patches will have a > subject line of that form. It may be better to search for "0/" and > ensure that it is immediately followed by an integer. Yes this could be moved. I wasn't sure about the risk of colliding with something else. > Also, it might be worth checking the message IDs to ensure that the > PATCH M/Ns all indeed are replies to PATCH 0/N. Doesn't that only work if mails [1..N] are reply to the cover ? Depending on the mailer and your option, this might not always be the case (I think). > >> - Support multiple patch series in a single run > Is this done? I would have expected that some buffering of messages > would be necessary, since you're writing a series of messages of the > form ... to the commits ... N>. > This is for git am. It handles the fact that an input may contain multiple series (expect them to be in order). When reaching patch N/N, it flushed the cover. >> TODO: >> - Add doc/comments >> - Add tests >> - Add a new "seperator" at the end of a cover letter. >> Right now I added a triple dash to all cover letter (manual or >> cover-at-tip) before shortlog/diff stat >> This allows manually written cover letters to be handle by git am >> --cover-at-tip without including the shortlog/diffstat but >> breaks compat with older git am as it is seen has a malformed patch. A new >> separator would solve that. > I think the triple dash works. I tried "git am" with a cover letter with > no triple dash, and it complains that the commit is empty anyway, so > compatibility with older git am might not be such a big issue. (With the > triple dash, it indeed complains about a malformed patch, as you > describe.) It kinda work but it makes the message difficult to understand for the user. And change the behaviour from the previous releases. Thanks for the feedback. Nicolas
Bug in "revision.c: --all adds HEAD from all worktrees" ?
Hi! I think there may be a regression caused by this change which means that "git fetch origin" doesn't work: commit d0c39a49ccb5dfe7feba4325c3374d99ab123c59 (refs/bisect/bad) Author: Nguyễn Thái Ngọc DuyDate: Wed Aug 23 19:36:59 2017 +0700 revision.c: --all adds HEAD from all worktrees $ git fetch origin fatal: bad object HEAD error: ssh://my_remote_host/reponame did not send all necessary objects I used git bisect to find the problem, and it seems pretty consistent. "git fetch" with the previous revision works fine. FWIW, I've got a lot of git worktrees associated with this repo, so that may be why it's failing. The remote repo is actually a git-p4 clone, so HEAD there actually ends up pointing at refs/remote/p4/master. Thanks, Luke
Re: [PATCH/RFC] Replace Free Software Foundation address in license notices
Junio C Hamano wrote: This change probably makes sense. From here on after applying the patch, we won't have to worry about updating these every time they move---not that they have moved often, though ;-) Indeed. It's thankfully a rare move. I imagine that's why it's somewhat common to find license text with the previous address long after the last move. (That and how boring licensing details are, in general.) compat/obstack.c| 5 ++--- ... ewah/ewok_rlw.h | 3 +-- git-gui/git-gui.sh | 3 +-- imap-send.c | 3 +-- ... 44 files changed, 69 insertions(+), 103 deletions(-) I've tried hard to keep the git-gui/ part as a separate project (it indeed is managed separately). I have been, and am still only pulling from its "upstream" repository (Pat, who is its project lead, Cc'ed), refaining from making changes that do not exist there at git://repo.or.cz/git-gui.git/ to the tree I publish. I'll separate the part from this patch that touches git-gui/* and try to arrange the next pull from git-gui repository would have the omitted part somehow. Given that the "upstream" seems to be inactive these days, we might want to change the way we manage that part of the tree, though. D'oh, I should have known that. Thanks for splitting this up. I was worried more minor things like legal details or changing code that we synced from another project (compat/regex or xdiff) might require some changes and didn't think enough about git-gui being separate. Thanks. Thank you too. It's always impressive to see how well you wear the maintainer hat. :) -- Todd ~~ A common mistake people make when trying to design something completely foolproof is to underestimate the ingenuity of complete fools. -- Douglas Adams
Re: [PATCH 02/30] merge-recursive: Fix logic ordering issue
On Fri, Nov 10, 2017 at 11:05 AM, Elijah Newrenwrote: > merge_trees() did a variety of work, including: > * Calling get_unmerged() to get unmerged entries > * Calling record_df_conflict_files() with all unmerged entries to > do some work to ensure we could handle D/F conflicts correctly > * Calling get_renames() to check for renames. > > An easily overlooked issue is that get_renames() can create more > unmerged entries and add them to the list, which have the possibility of > being involved in D/F conflicts. I presume these are created via insert_stage_data called in get_renames, when the path entry is not found? > So the call to > record_df_conflict_files() should really be moved after all the rename > detection. I didn't come up with any testcases demonstrating any bugs > with the old ordering, but I suspect there were some for both normal > renames and for directory renames. Fix the ordering. It is hard to trace this down, though looking at 3af244caa8 (Cumulative update of merge-recursive in C, 2006-07-27) may help us reason about it. How would a bug look like? > > Signed-off-by: Elijah Newren > --- > merge-recursive.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/merge-recursive.c b/merge-recursive.c > index 1d3f8f0d22..52521faf09 100644 > --- a/merge-recursive.c > +++ b/merge-recursive.c > @@ -1981,10 +1981,10 @@ int merge_trees(struct merge_options *o, > get_files_dirs(o, merge); > > entries = get_unmerged(); > - record_df_conflict_files(o, entries); > re_head = get_renames(o, head, common, head, merge, entries); > re_merge = get_renames(o, merge, common, head, merge, > entries); > clean = process_renames(o, re_head, re_merge); > + record_df_conflict_files(o, entries); > if (clean < 0) > goto cleanup; > for (i = entries->nr-1; 0 <= i; i--) { > -- > 2.15.0.5.g9567be9905 >
Re: [RFC 0/3] Add support for --cover-at-tip
On Mon, 13 Nov 2017 18:13:27 +0100 Nicolas Morey-Chaisemartinwrote: > v2: > - Enhance mailinfo to parse patch series id from subject > - Detect cover using mailinfo parsed ids in git am I noticed that this was done in the patch set by searching for "PATCH" - that is probably quite error-prone as not all patches will have a subject line of that form. It may be better to search for "0/" and ensure that it is immediately followed by an integer. Also, it might be worth checking the message IDs to ensure that the PATCH M/Ns all indeed are replies to PATCH 0/N. > - Support multiple patch series in a single run Is this done? I would have expected that some buffering of messages would be necessary, since you're writing a series of messages of the form ... to the commits > TODO: > - Add doc/comments > - Add tests > - Add a new "seperator" at the end of a cover letter. > Right now I added a triple dash to all cover letter (manual or > cover-at-tip) before shortlog/diff stat > This allows manually written cover letters to be handle by git am > --cover-at-tip without including the shortlog/diffstat but > breaks compat with older git am as it is seen has a malformed patch. A new > separator would solve that. I think the triple dash works. I tried "git am" with a cover letter with no triple dash, and it complains that the commit is empty anyway, so compatibility with older git am might not be such a big issue. (With the triple dash, it indeed complains about a malformed patch, as you describe.)
Re: [PATCH 01/30] Tighten and correct a few testcases for merging and cherry-picking
On Fri, Nov 10, 2017 at 11:05 AM, Elijah Newrenwrote: > t3501 had a testcase originally added ... goes and looks ... "in 05f2dfb965 (cherry-pick: demonstrate a segmentation fault, 2016-11-26)" would have helped me here in the commit message. > to ensure cherry-pick wouldn't > segfault when working with a dirty file involved in a rename. While > the segfault was fixed, there was another problem this test demonstrated: > namely, that git would overwrite a dirty file involved in a rename. > Further, the test encoded a "successful merge" and overwriting of this > file as correct behavior. Modify the test so that it would still catch > the segfault, but to require the correct behavior. As the correct behavior is not yet implemented, mark it as test_expect_failure, too. (probably this reads implicit) > > t7607 had a test ... added in 30fd3a5425 (merge overwrites unstaged changes in renamed file, 2012-04-15) ... > specific to looking for a merge overwriting a dirty file > involved in a rename, but it too actually encoded what I would term > incorrect behavior: it expected the merge to succeed. Fix that, and add > a few more checks to make sure that the merge really does produce the > expected results. > > Signed-off-by: Elijah Newren > --- > t/t3501-revert-cherry-pick.sh | 7 +-- > t/t7607-merge-overwrite.sh| 5 - > 2 files changed, 9 insertions(+), 3 deletions(-) > > diff --git a/t/t3501-revert-cherry-pick.sh b/t/t3501-revert-cherry-pick.sh > index 4f2a263b63..783bdbf59d 100755 > --- a/t/t3501-revert-cherry-pick.sh > +++ b/t/t3501-revert-cherry-pick.sh > @@ -141,7 +141,7 @@ test_expect_success 'cherry-pick "-" works with > arguments' ' > test_cmp expect actual > ' > > -test_expect_success 'cherry-pick works with dirty renamed file' ' > +test_expect_failure 'cherry-pick works with dirty renamed file' ' > test_commit to-rename && > git checkout -b unrelated && > test_commit unrelated && > @@ -150,7 +150,10 @@ test_expect_success 'cherry-pick works with dirty > renamed file' ' > test_tick && > git commit -m renamed && > echo modified >renamed && > - git cherry-pick refs/heads/unrelated > + test_must_fail git cherry-pick refs/heads/unrelated >out && > + test_i18ngrep "Refusing to lose dirty file at renamed" out && > + test $(git rev-parse :0:renamed) = $(git rev-parse HEAD^:to-rename.t) > && > + grep -q "^modified$" renamed > ' > > test_done > diff --git a/t/t7607-merge-overwrite.sh b/t/t7607-merge-overwrite.sh > index 9444d6a9b9..00617dadf8 100755 > --- a/t/t7607-merge-overwrite.sh > +++ b/t/t7607-merge-overwrite.sh > @@ -97,7 +97,10 @@ test_expect_failure 'will not overwrite unstaged changes > in renamed file' ' > git mv c1.c other.c && > git commit -m rename && > cp important other.c && > - git merge c1a && > + test_must_fail git merge c1a >out && > + test_i18ngrep "Refusing to lose dirty file at other.c" out && > + test -f other.c~HEAD && > + test $(git hash-object other.c~HEAD) = $(git rev-parse c1a:c1.c) && > test_cmp important other.c Code looks good, Thanks, Stefan
Re:
To subscribe to the git mailing list, send the email to majord...@vger.kernel.org, not the mailing list itself. On Sat, Nov 11, 2017 at 6:21 PM,wrote: > subscribe git
Re: [PATCH v3 4/4] Switch empty tree and blob lookups to use hash abstraction
On Sun, Nov 12, 2017 at 1:28 PM, brian m. carlsonwrote: > Switch the uses of empty_tree_oid and empty_blob_oid to use the > current_hash abstraction that represents the current hash algorithm in > use. > > Signed-off-by: brian m. carlson > --- Thanks for writing this series, all four patches look good to me. Thanks, Stefan
Re: cherry-pick very slow on big repository
On Mon, Nov 13, 2017 at 3:22 AM, Peter Kreftingwrote: > Elijah Newren: > >> I would be very interested to hear how my rename detection performance >> patches work for you; this kind of usecase was the exact one it was designed >> to help the most. See >> https://public-inbox.org/git/20171110222156.23221-1-new...@gmail.com/ > > I'd be happy to try them out. Is there a public repo where I can pull these > patches from instead of trying to apply them manually, as there are several > patch series involved here? Sure, take a look at the big-repo-small-cherry-pick branch of https://github.com/newren/git
[PATCH V2] config: add --expiry-date
Description: This patch adds a new option to the config command. Enables flag --expiry-date as a data-type to covert date-strings to timestamps when reading from config files (GET). This flag is ignored on write (SET) because the date-string is stored in config without performing any normalization. A few test cases are also created since this is a new feature. Motivation: A parse_expiry_date() function already existed for api calls, this patch simply allows the function to be used from the command line. Update: Added suggestions, documentation, relative time test case and test helper function to print out timestamps for comparison. Updated reflog.c to avoid function duplication. Signed-off-by: Haaris--- Documentation/git-config.txt | 5 + builtin/config.c | 10 +- builtin/reflog.c | 14 ++ config.c | 9 + config.h | 1 + t/helper/test-date.c | 12 t/t1300-repo-config.sh | 30 ++ 7 files changed, 68 insertions(+), 13 deletions(-) diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt index 4edd09fc6..14da5fc15 100644 --- a/Documentation/git-config.txt +++ b/Documentation/git-config.txt @@ -180,6 +180,11 @@ See also <>. value (but you can use `git config section.variable ~/` from the command line to let your shell do the expansion). +--expiry-date:: + `git config` will ensure that the output is converted from + a fixed or relative date-string to a timestamp. This option + has no effect when setting the value. + -z:: --null:: For all options that output values and/or keys, always diff --git a/builtin/config.c b/builtin/config.c index d13daeeb5..afdb02191 100644 --- a/builtin/config.c +++ b/builtin/config.c @@ -52,6 +52,7 @@ static int show_origin; #define TYPE_INT (1<<1) #define TYPE_BOOL_OR_INT (1<<2) #define TYPE_PATH (1<<3) +#define TYPE_EXPIRY_DATE (1<<4) static struct option builtin_config_options[] = { OPT_GROUP(N_("Config file location")), @@ -80,6 +81,7 @@ static struct option builtin_config_options[] = { OPT_BIT(0, "int", , N_("value is decimal number"), TYPE_INT), OPT_BIT(0, "bool-or-int", , N_("value is --bool or --int"), TYPE_BOOL_OR_INT), OPT_BIT(0, "path", , N_("value is a path (file or directory name)"), TYPE_PATH), + OPT_BIT(0, "expiry-date", , N_("value is an expiry date"), TYPE_EXPIRY_DATE), OPT_GROUP(N_("Other")), OPT_BOOL('z', "null", _null, N_("terminate values with NUL byte")), OPT_BOOL(0, "name-only", _values, N_("show variable names only")), @@ -159,6 +161,11 @@ static int format_config(struct strbuf *buf, const char *key_, const char *value return -1; strbuf_addstr(buf, v); free((char *)v); + } else if (types == TYPE_EXPIRY_DATE) { + timestamp_t t; + if(git_config_expiry_date(, key_, value_) < 0) + return -1; + strbuf_addf(buf, "%"PRItime, t); } else if (value_) { strbuf_addstr(buf, value_); } else { @@ -273,12 +280,13 @@ static char *normalize_value(const char *key, const char *value) if (!value) return NULL; - if (types == 0 || types == TYPE_PATH) + if (types == 0 || types == TYPE_PATH || types == TYPE_EXPIRY_DATE) /* * We don't do normalization for TYPE_PATH here: If * the path is like ~/foobar/, we prefer to store * "~/foobar/" in the config file, and to expand the ~ * when retrieving the value. +* Also don't do normalization for expiry dates. */ return xstrdup(value); if (types == TYPE_INT) diff --git a/builtin/reflog.c b/builtin/reflog.c index ab31a3b6a..223372531 100644 --- a/builtin/reflog.c +++ b/builtin/reflog.c @@ -416,16 +416,6 @@ static struct reflog_expire_cfg *find_cfg_ent(const char *pattern, size_t len) return ent; } -static int parse_expire_cfg_value(const char *var, const char *value, timestamp_t *expire) -{ - if (!value) - return config_error_nonbool(var); - if (parse_expiry_date(value, expire)) - return error(_("'%s' for '%s' is not a valid timestamp"), -value, var); - return 0; -} - /* expiry timer slot */ #define EXPIRE_TOTAL 01 #define EXPIRE_UNREACH 02 @@ -443,11 +433,11 @@ static int reflog_expire_config(const char *var, const char *value, void *cb) if (!strcmp(key, "reflogexpire")) { slot = EXPIRE_TOTAL; - if (parse_expire_cfg_value(var, value, )) + if (git_config_expiry_date(, var, value))
Re: man page for "git-worktree" is a bit confusing WRT "prune"
On Mon, Nov 13, 2017 at 9:48 AM, Robert P. J. Daywrote: > once more, into the man pages ... "git worktree" seems like a fairly > simple command, but there is some confusion about the function of > > $ git worktree prune > > the normal meaning of "prune" (certainly with git commands) is to > actually delete some content, and the initial impression of this > command is that it will delete an actual worktree. however, further > reading reveals: > > " ... or you can run git worktree prune in the main or any linked > working tree to clean up any stale administrative files." > > ah, so one learns that the subcommand "prune" does *not* do any > actual pruning as people would *normally* understand it, it simply > deletes the administrative information about an already-deleted > worktree, do i read that correctly? Yes. This usage is consistent with "git remote prune" which removes administrative information about local branches which have already been deleted on the remote side. > that's emphasized further down in the actual definition of "prune": > > prune > Prune working tree information in $GIT_DIR/worktrees. > > but perhaps that explanation could be extended to say it only works on > already-deleted trees, since that's certainly not clear from that > single sentence. As originally implemented, git-worktree would detect deleted or relocated worktrees and prune or update the administrative information automatically. So, "prune" was more a behind-the-scenes implementation detail rather than an important user-facing command. However, the implementation and semantics of that automatic behavior were not quite robust and ended up leaving things in a slightly corrupted state (if I recall correctly), though the corruption was easily corrected by hand. As a consequence, the automatic behavior was retired while the general implementation of git-worktree "cooked", with the idea that it could be revisited later, with the result that "prune" became more user-facing than originally intended. The above description could be extended with more information. An alternative would be to point the reader at the "DETAILS" section as is done already for "prune" in "DISCUSSION". > finally, the prune "--expire" option is truly confusing: > > --expire > With prune, only expire unused working trees older than . > > suddenly, we encounter the verb "expire", which means ... what? how > does "expiring" a worktree differ from "pruning" a worktree? and what > makes a worktree "unused"? the normal meaning of "unused" is that you > haven't, you know, *used* it lately. in this context, though, does it > mean deleted? and if it means deleted, what does it mean for it to be > older than some time if it's already gone? > > thoughts? This dates back to the original behavior of automatically pruning administrative information for deleted worktrees. As discussed elsewhere in the document, a worktree may be placed on some removable device (USB drive, memory stick, etc.) or network share which isn't always mounted. The "expire time" provides such not-necessarily-mounted worktrees a grace period before being pruned automatically. You wouldn't want your worktree administrative information erased automatically when invoking some git-worktree command -- say "git worktree list" -- simply because you forgot to plug your memory stick back into the computer; the grace period protects against this sort of lossage. As with "prune", originally, the grace period was more a behind-the-scenes detail than a user-facing feature. Nevertheless, it's still useful; you might forget to plug in your memory stick before invoking "git worktree prune" manually. The term "unused" is unfortunate. A better description would likely be welcome.
[RFC 3/3] log: add an option to generate cover letter from a branch tip
TODO: figure out defaults, add a config option, move tip detection to specific function Signed-off-by: Nicolas Morey-Chaisemartin--- Documentation/git-format-patch.txt | 4 builtin/log.c | 44 +- 2 files changed, 38 insertions(+), 10 deletions(-) diff --git a/Documentation/git-format-patch.txt b/Documentation/git-format-patch.txt index 6cbe462a7..0ac9d4b71 100644 --- a/Documentation/git-format-patch.txt +++ b/Documentation/git-format-patch.txt @@ -228,6 +228,10 @@ feeding the result to `git send-email`. containing the branch description, shortlog and the overall diffstat. You can fill in a description in the file before sending it out. +--[no-]cover-letter-at-tip:: + Use the tip of the series as a cover letter if it is an empty commit. +If no cover-letter is to be sent, the tip is ignored. + --notes[=]:: Append the notes (see linkgit:git-notes[1]) for the commit after the three-dash line. diff --git a/builtin/log.c b/builtin/log.c index 6c1fa896a..292626482 100644 --- a/builtin/log.c +++ b/builtin/log.c @@ -986,11 +986,11 @@ static void make_cover_letter(struct rev_info *rev, int use_stdout, struct commit *origin, int nr, struct commit **list, const char *branch_name, - int quiet) + int quiet, + struct commit *cover_at_tip_commit) { const char *committer; - const char *body = "*** SUBJECT HERE ***\n\n*** BLURB HERE ***\n"; - const char *msg; + const char *body = "*** SUBJECT HERE ***\n\n*** BLURB HERE ***\n\n"; struct shortlog log; struct strbuf sb = STRBUF_INIT; int i; @@ -1021,17 +1021,21 @@ static void make_cover_letter(struct rev_info *rev, int use_stdout, if (!branch_name) branch_name = find_branch_name(rev); - msg = body; pp.fmt = CMIT_FMT_EMAIL; pp.date_mode.type = DATE_RFC2822; pp.rev = rev; pp.print_email_subject = 1; - pp_user_info(, NULL, , committer, encoding); - pp_title_line(, , , encoding, need_8bit_cte); - pp_remainder(, , , 0); - add_branch_description(, branch_name); - fprintf(rev->diffopt.file, "%s\n", sb.buf); + if (!cover_at_tip_commit) { + pp_user_info(, NULL, , committer, encoding); + pp_title_line(, , , encoding, need_8bit_cte); + pp_remainder(, , , 0); + } else { + pretty_print_commit(, cover_at_tip_commit, ); + } + add_branch_description(, branch_name); + fprintf(rev->diffopt.file, "%s", sb.buf); + fprintf(rev->diffopt.file, "---\n", sb.buf); strbuf_release(); shortlog_init(); @@ -1409,6 +1413,8 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix) int just_numbers = 0; int ignore_if_in_upstream = 0; int cover_letter = -1; + int cover_at_tip = -1; + struct commit *cover_at_tip_commit = NULL; int boundary_count = 0; int no_binary_diff = 0; int zero_commit = 0; @@ -1437,6 +1443,8 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix) N_("print patches to standard out")), OPT_BOOL(0, "cover-letter", _letter, N_("generate a cover letter")), + OPT_BOOL(0, "cover-at-tip", _at_tip, + N_("fill the cover letter with the tip of the branch")), OPT_BOOL(0, "numbered-files", _numbers, N_("use simple number sequence for output file names")), OPT_STRING(0, "suffix", _patch_suffix, N_("sfx"), @@ -1698,6 +1706,21 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix) if (ignore_if_in_upstream && has_commit_patch_id(commit, )) continue; + if (!nr && cover_at_tip == 1 && !cover_at_tip_commit) { + /* Check that it is a candidate to be a cover at tip +* Meaning: +* - a single parent (merge commits are not eligible) +* - tree oid == parent->tree->oid (no diff to the tree) +*/ + if (commit->parents && !commit->parents->next && + !oidcmp(>tree->object.oid, + >parents->item->tree->object.oid)) { + cover_at_tip_commit = commit; + continue; + } else { + cover_at_tip = 0; + } + } nr++; REALLOC_ARRAY(list, nr);
[RFC 1/3] mailinfo: extract patch series id
Extract the patch ID and series length from the [PATCH N/M] prefix in the mail header Signed-off-by: Nicolas Morey-Chaisemartin--- mailinfo.c | 35 +++ mailinfo.h | 2 ++ 2 files changed, 37 insertions(+) diff --git a/mailinfo.c b/mailinfo.c index a89db22ab..2ab9d446d 100644 --- a/mailinfo.c +++ b/mailinfo.c @@ -308,6 +308,39 @@ static void cleanup_subject(struct mailinfo *mi, struct strbuf *subject) if (!pos) break; remove = pos - subject->buf + at + 1; + if (7 <= remove && + memmem(subject->buf + at, remove, "PATCH", 5)){ + /* +* Look for a N/M series identifier at +* the end of the brackets +*/ + int is_series = 1; + int ret, num, total; + int pt = at + remove - 2; + + if (!isdigit(subject->buf[pt])) + is_series = 0; + else { + while(isdigit(subject->buf[--pt])); + } + + if(is_series && subject->buf[pt--] != '/') + is_series = 0; + + if (!isdigit(subject->buf[pt])) + is_series = 0; + if (is_series) + while(isdigit(subject->buf[--pt])); + + pt++; + + ret = sscanf(subject->buf + pt, "%d/%d]", , ); + if (ret == 2){ + mi->series_id = num; + mi->series_len = total; + } + } + if (!mi->keep_non_patch_brackets_in_subject || (7 <= remove && memmem(subject->buf + at, remove, "PATCH", 5))) @@ -1154,6 +1187,8 @@ void setup_mailinfo(struct mailinfo *mi) mi->header_stage = 1; mi->use_inbody_headers = 1; mi->content_top = mi->content; + mi->series_id = -1; + mi->series_len = -1; git_config(git_mailinfo_config, mi); } diff --git a/mailinfo.h b/mailinfo.h index 04a25351d..bd4f7c9e0 100644 --- a/mailinfo.h +++ b/mailinfo.h @@ -21,6 +21,8 @@ struct mailinfo { struct strbuf **content_top; struct strbuf charset; char *message_id; + int series_id;/* Id of the patch within a patch series. -1 if not a patch series */ + int series_len; /* Length of the patch series. -1 if not a patch series */ enum { TE_DONTCARE, TE_QP, TE_BASE64 } transfer_encoding; -- 2.15.0.169.g3d3eebb67.dirty
[RFC 2/3] am: semi working --cover-at-tip
Issue with empty patch detection Signed-off-by: Nicolas Morey-Chaisemartin--- builtin/am.c | 143 --- 1 file changed, 126 insertions(+), 17 deletions(-) diff --git a/builtin/am.c b/builtin/am.c index 92c485350..702cbf8e0 100644 --- a/builtin/am.c +++ b/builtin/am.c @@ -111,6 +111,11 @@ struct am_state { char *msg; size_t msg_len; + /* Series metadata */ + int series_id; + int series_len; + int cover_id; + /* when --rebasing, records the original commit the patch came from */ struct object_id orig_commit; @@ -131,6 +136,8 @@ struct am_state { int committer_date_is_author_date; int ignore_date; int allow_rerere_autoupdate; + int cover_at_tip; + int applying_cover; const char *sign_commit; int rebasing; }; @@ -160,6 +167,7 @@ static void am_state_init(struct am_state *state) if (!git_config_get_bool("commit.gpgsign", )) state->sign_commit = gpgsign ? "" : NULL; + } /** @@ -432,6 +440,20 @@ static void am_load(struct am_state *state) read_state_file(, state, "utf8", 1); state->utf8 = !strcmp(sb.buf, "t"); + read_state_file(, state, "cover-at-tip", 1); + state->cover_at_tip = !strcmp(sb.buf, "t"); + + if (state->cover_at_tip) { + read_state_file(, state, "series_id", 1); + state->series_id = strtol(sb.buf, NULL, 10); + + read_state_file(, state, "series_len", 1); + state->series_len = strtol(sb.buf, NULL, 10); + + read_state_file(, state, "cover_id", 1); + state->cover_id = strtol(sb.buf, NULL, 10); + } + if (file_exists(am_path(state, "rerere-autoupdate"))) { read_state_file(, state, "rerere-autoupdate", 1); state->allow_rerere_autoupdate = strcmp(sb.buf, "t") ? @@ -1020,6 +1042,7 @@ static void am_setup(struct am_state *state, enum patch_format patch_format, write_state_bool(state, "quiet", state->quiet); write_state_bool(state, "sign", state->signoff); write_state_bool(state, "utf8", state->utf8); + write_state_bool(state, "cover-at-tip", state->cover_at_tip); if (state->allow_rerere_autoupdate) write_state_bool(state, "rerere-autoupdate", @@ -1076,6 +1099,12 @@ static void am_setup(struct am_state *state, enum patch_format patch_format, delete_ref(NULL, "ORIG_HEAD", NULL, 0); } + if (state->cover_at_tip) { + write_state_count(state, "series_id", state->series_id); + write_state_count(state, "series_len", state->series_len); + write_state_count(state, "cover_id", state->cover_id); + } + /* * NOTE: Since the "next" and "last" files determine if an am_state * session is in progress, they should be written last. @@ -1088,13 +1117,9 @@ static void am_setup(struct am_state *state, enum patch_format patch_format, } /** - * Increments the patch pointer, and cleans am_state for the application of the - * next patch. - */ -static void am_next(struct am_state *state) + * Cleans am_state. + */static void am_clean(struct am_state *state) { - struct object_id head; - FREE_AND_NULL(state->author_name); FREE_AND_NULL(state->author_email); FREE_AND_NULL(state->author_date); @@ -1106,14 +1131,6 @@ static void am_next(struct am_state *state) oidclr(>orig_commit); unlink(am_path(state, "original-commit")); - - if (!get_oid("HEAD", )) - write_state_text(state, "abort-safety", oid_to_hex()); - else - write_state_text(state, "abort-safety", ""); - - state->cur++; - write_state_count(state, "next", state->cur); } /** @@ -1274,6 +1291,7 @@ static int parse_mail(struct am_state *state, const char *mail) fclose(mi.input); fclose(mi.output); + /* Extract message and author information */ fp = xfopen(am_path(state, "info"), "r"); while (!strbuf_getline_lf(, fp)) { @@ -1298,9 +1316,30 @@ static int parse_mail(struct am_state *state, const char *mail) goto finish; } - if (is_empty_file(am_path(state, "patch"))) { - printf_ln(_("Patch is empty.")); - die_user_resolve(state); + if (!state->applying_cover) { + + state->series_id = mi.series_id; + state->series_len = mi.series_len; + + if (state->cover_at_tip) { + write_state_count(state, "series_id", state->series_id); + write_state_count(state, "series_len", state->series_len); + write_state_count(state, "cover_id", state->cover_id); + } + + if (mi.series_id == 0){ +
[RFC 0/3] Add support for --cover-at-tip
v2: - Enhance mailinfo to parse patch series id from subject - Detect cover using mailinfo parsed ids in git am - Support multiple patch series in a single run TODO: - Add doc/comments - Add tests - Add a new "seperator" at the end of a cover letter. Right now I added a triple dash to all cover letter (manual or cover-at-tip) before shortlog/diff stat This allows manually written cover letters to be handle by git am --cover-at-tip without including the shortlog/diffstat but breaks compat with older git am as it is seen has a malformed patch. A new separator would solve that. Note: Cover letter automatically generated with --cover-at-tip ;) Signed-off-by: Nicolas Morey-Chaisemartin--- Nicolas Morey-Chaisemartin (3): mailinfo: extract patch series id am: semi working --cover-at-tip log: add an option to generate cover letter from a branch tip Documentation/git-format-patch.txt | 4 ++ builtin/am.c | 143 - builtin/log.c | 44 +--- mailinfo.c | 35 + mailinfo.h | 2 + 5 files changed, 201 insertions(+), 27 deletions(-) -- 2.15.0.169.g3d3eebb67.dirty
Re: [PATCH] link_alt_odb_entries: make empty input a noop
Jeff King wrote: > This should make Joey's immediate pain go away, though only by papering > it over. I tend to agree that we shouldn't be looking at $PWD at all > here. I've confirmed that Jeff's patch fixes the case I was having trouble with. -- see shy jo signature.asc Description: PGP signature
[no subject]
sup Git http://bit.ly/2zz7jZe Yours Truly Cwestin
Re: [PATCH v1 1/4] fastindex: speed up index load through parallelization
On 11/9/2017 11:46 PM, Junio C Hamano wrote: Ben Peartwrites: To make this work for V4 indexes, when writing the index, it periodically "resets" the prefix-compression by encoding the current entry as if the path name for the previous entry is completely different and saves the offset of that entry in the IEOT. Basically, with V4 indexes, it generates offsets into blocks of prefix-compressed entries. You specifically mentioned about this change in your earlier correspondence on this series, and it reminds me of Shawn's reftable proposal that also is heavily depended on prefix compression with occasional reset to allow scanning from an entry in the middle. I find it a totally sensible design choice. Great! I didn't realize there was already precedent for this in other patches. I guess good minds think alike... :) To enable reading the IEOT extension before reading all the variable length cache entries and other extensions, the IEOT is written last, right before the trailing SHA1. OK, we have already closed the door for further enhancements to place something other than the index extensions after this block by mistakenly made it the rule that the end of the series of extended sections must coincide with the beginning of the trailing checksum, so it does not sound all that bad to insist on this particular one to be the last, I guess. But see below... The format of that extension has the signature bytes and size at the beginning (like a normal extension) as well as at the end in reverse order to enable parsing the extension by seeking back from the end of the file. I think this is a reasonable workaround to allow a single extension that needs to be read before the main index is loaded. But I'd suggest taking this approach one step further. Instead, what if we introduce a new extension EOIE ("end of index entries") whose sole purpose is to sit at the end of the series of extensions and point at the beginning of the index extension section of the file, to tell you where to seek in order to skip the main index? That way, you can - seek to the end of the index file; - go backward skiping the trailing file checksum; - now you might be at the end of the EOIE extension. seek back necessary number of bytes and verify EOIE header, pick up the recorded file offset of the beginning of the extensions section. - The 4-byte sequence you found may happen to match EOIE but that is not enough to be sure that you indeed have such an extension. So the following must be done carefully, allowing the possibility that there wasn't any EOIE extension at the end. Seek back to that offset, and repeat these three steps to skip over all extensions: - read the (possible) 4-byte header - read the (possible) extsize (validate that this is a sane value) - skip that many bytes until you come back to the location you assumed that you found your EOIE header, to make sure you did not get fooled by bytes that happened to look like one. Some "extsize" you picked up during that process may lead you beyond the end of the index file, which would be a sure sign that what you found at the end of the index file was *not* the EOIE extension but a part of some other extension who happened to have these four bytes at the right place. I'm doing this careful examination and verification in the patch already. Please look closely at read_ieot_extension() to make sure there isn't a test I should be doing but missed. which would be a lot more robust way to allow any extensions to be read before the main body of the index. And a lot more importantly, we will leave the door open to allow more than one index extensions that we would prefer to read before reading the main body if we do it this way, because we can easily skip things over without spending cycles once we have a robust way to find where the end of the main index is. After all, the reason you placed IEOT at the end is not because you wanted to have it at the very end. You only wanted to be able to find where it is without having to parse the variable-length records of the main index. And there may be other index extensions that wants to be readable without reading the main part just like IEOT, and we do not want to close the door for them. The proposed format for extensions (ie having both a header and a footer with name and size) in the current patch already enables having multiple extensions that can be parsed forward or backward. Any extensions that would want to be parse-able in reverse would just all need to be written one after the other after right before the trailing SHA1 (and of course, include the proper footer). The same logic that parses the IEOT extension could be extended to continue looking backwards through the file parsing extensions until it encounters an unknown signature, invalid size, etc. That said, the IEOT is