Re: no mountable file systems

[Christian Couder]

On Mon, Nov 13, 2017 at 10:35 PM, Louis Gruand
 wrote:
> 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

[Junio C Hamano]

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

[Christian Couder]

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)

[Junio C Hamano]

Kaartic Sivaraam  writes:

>> * 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

[Junio C Hamano]

Nicolas Morey-Chaisemartin  writes:

> - 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

[Junio C Hamano]

Nicolas Morey-Chaisemartin  writes:

>   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

[Jeff King]

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

[Junio C Hamano]

Nicolas Morey-Chaisemartin  writes:

> 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)

[Kaartic Sivaraam]

* 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()

[Junio C Hamano]

Elijah Newren  writes:

> + 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

[Kaartic Sivaraam]

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

[Junio C Hamano]

Elijah Newren  writes:

> 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

[Todd Zullinger]

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 Hamano]

Junio C Hamano  writes:

> 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

[Junio C Hamano]

Elijah Newren  writes:

> 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

[Junio C Hamano]

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
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

[Junio C Hamano]

Elijah Newren  writes:

> 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)

[Junio C Hamano]

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

[Junio C Hamano]

Todd Zullinger  writes:

>> 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"

[Eric Sunshine]

[+cc:Duy]

On Mon, Nov 13, 2017 at 4:06 PM, Robert P. J. Day  wrote:
> 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

[Junio C Hamano]

Ann T Ropea  writes:

> 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

[Todd Zullinger]

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

[Junio C Hamano]

Ann T Ropea  writes:

> 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

[Todd Zullinger]

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

[Junio C Hamano]

Todd Zullinger  writes:

> 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

[Junio C Hamano]

Charles Bailey  writes:

>> > 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

[Junio C Hamano]

Joey Hess  writes:

> 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

[hsed]

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/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

[Junio C Hamano]

Stefan Beller  writes:

> 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

[Elijah Newren]

On Mon, Nov 13, 2017 at 5:21 PM, Stefan Beller  wrote:
> 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

[Elijah Newren]

On Mon, Nov 13, 2017 at 4:25 PM, Stefan Beller  wrote:
> 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

[Junio C Hamano]

Elijah Newren  writes:

> 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

[Stefan Beller]

On Mon, Nov 13, 2017 at 4:57 PM, Elijah Newren  wrote:
> 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

[Elijah Newren]

On Mon, Nov 13, 2017 at 4:15 PM, Stefan Beller  wrote:
> 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

[Junio C Hamano]

Elijah Newren  writes:

>   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

[Junio C Hamano]

Phillip Wood  writes:

> 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

[Junio C Hamano]

Ben Peart  writes:

> 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

[Elijah Newren]

On Mon, Nov 13, 2017 at 3:25 PM, Stefan Beller  wrote:
> 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

[Elijah Newren]

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.

>> +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

[Stefan Beller]

On Fri, Nov 10, 2017 at 11:05 AM, Elijah Newren  wrote:

> +# 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

[Stefan Beller]

On Fri, Nov 10, 2017 at 11:05 AM, Elijah Newren  wrote:
> 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

[Stefan Beller]

On Fri, Nov 10, 2017 at 11:05 AM, Elijah Newren  wrote:
> 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

[Stefan Beller]

On Fri, Nov 10, 2017 at 11:05 AM, Elijah Newren  wrote:
> 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

[Stefan Beller]

On Mon, Nov 13, 2017 at 3:39 PM, Elijah Newren  wrote:
> 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

[Elijah Newren]

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.

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

[Stefan Beller]

On Fri, Nov 10, 2017 at 11:05 AM, Elijah Newren  wrote:
> 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

[Stefan Beller]

On Fri, Nov 10, 2017 at 11:05 AM, Elijah Newren  wrote:
> 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

[Stefan Beller]

On Mon, Nov 13, 2017 at 2:57 PM, Elijah Newren  wrote:
>
> 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

[Santiago Torres]

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

[Santiago Torres]

 
> 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

[Elijah Newren]

On Mon, Nov 13, 2017 at 1:06 PM, Stefan Beller  wrote:
> 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

[Todd Zullinger]

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

[Ann T Ropea]

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

[Ann T Ropea]

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

[Ann T Ropea]

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

[Ann T Ropea]

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").

[Ann T Ropea]

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

[Ann T Ropea]

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

[Elijah Newren]

Thanks for the reviews!

On Mon, Nov 13, 2017 at 1:33 PM, Jonathan Tan  wrote:
> 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

[Santiago Torres]

 
> 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" ?

[Stefan Beller]

On Mon, Nov 13, 2017 at 2:03 PM, Luke Diamand  wrote:
> 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

[Stefan Beller]

On Mon, Nov 13, 2017 at 2:04 PM, Elijah Newren  wrote:

> 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

[GORD CH]

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

[Elijah Newren]

Thanks for the reviews!

On Mon, Nov 13, 2017 at 11:48 AM, Stefan Beller  wrote:
> 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

[Stefan Beller]

On Fri, Nov 10, 2017 at 11:05 AM, Elijah Newren  wrote:
> 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" ?

[Luke Diamand]

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.

[buglet] gitk and git cherry-pick --abort interaction

[Chris Packham]

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

[Louis Gruand]

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

[Jonathan Tan]

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.

Recovering from gc errors

[Marc Branchaud]

(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()

[Todd Zullinger]

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

[Martin Ågren]

On 11 November 2017 at 00:13, Joel Teichroeb  wrote:
> 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

[Todd Zullinger]

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"

[Robert P. J. Day]

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

[Stefan Beller]

On Fri, Nov 10, 2017 at 11:05 AM, Elijah Newren  wrote:
> 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

[Jonathan Tan]

On Mon, 13 Nov 2017 12:15:58 -0800
Elijah Newren  wrote:

> 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

[Elijah Newren]

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

[Elijah Newren]

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

[Elijah Newren]

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

[Elijah Newren]

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

[Elijah Newren]

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

[Elijah Newren]

Thanks for the reviews and suggestions!

On Sun, Nov 12, 2017 at 9:24 PM, Junio C Hamano  wrote:
> 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

[Nicolas Morey-Chaisemartin]

Le 13/11/2017 à 20:40, Jonathan Tan a écrit :
> On Mon, 13 Nov 2017 18:13:27 +0100
> Nicolas Morey-Chaisemartin  wrote:
>
>> 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" ?

[Luke Diamand]

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

Re: [PATCH/RFC] Replace Free Software Foundation address in license notices

[Todd Zullinger]

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

[Stefan Beller]

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?

> 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

[Jonathan Tan]

On Mon, 13 Nov 2017 18:13:27 +0100
Nicolas Morey-Chaisemartin  wrote:

> 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

[Stefan Beller]

On Fri, Nov 10, 2017 at 11:05 AM, Elijah Newren  wrote:
> 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:

[Stefan Beller]

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

[Stefan Beller]

On Sun, Nov 12, 2017 at 1:28 PM, brian m. carlson
 wrote:
> 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

[Elijah Newren]

On Mon, Nov 13, 2017 at 3:22 AM, Peter Krefting  wrote:
> 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

[hsed]

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"

[Eric Sunshine]

On Mon, Nov 13, 2017 at 9:48 AM, Robert P. J. Day  wrote:
>   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

[Nicolas Morey-Chaisemartin]

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

[Nicolas Morey-Chaisemartin]

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

[Nicolas Morey-Chaisemartin]

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

[Nicolas Morey-Chaisemartin]

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

[Joey Hess]

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]

[cwestin]

sup Git



http://bit.ly/2zz7jZe



Yours Truly

Cwestin

Re: [PATCH v1 1/4] fastindex: speed up index load through parallelization

[Ben Peart]

On 11/9/2017 11:46 PM, Junio C Hamano wrote:

Ben Peart  writes:


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