error: unable to create file: Illegal byte sequence

[Brian Buchalter]

I am attempting to repair a git repo which has an illegal byte
sequence but am not sure how to proceed. Steps to reproduce: `git
clone https://github.com/christopherpow/nes-test-roms.git` results in:

```
Cloning into 'nes-test-roms'...
remote: Counting objects: 1049, done.
remote: Total 1049 (delta 0), reused 0 (delta 0), pack-reused 1049
Receiving objects: 100% (1049/1049), 5.23 MiB | 8.97 MiB/s, done.
Resolving deltas: 100% (406/406), done.
error: unable to create file other/Duelito - L�eme.txt: Illegal byte sequence
fatal: unable to checkout working tree
warning: Clone succeeded, but checkout failed.
You can inspect what was checked out with 'git status'
and retry the checkout with 'git checkout -f HEAD'
```

Running macOS 10.13.3, git version 2.16.1

Thank you!

Re: [PATCH 0/2] Refactor hash search with fanout table

[Derrick Stolee]

On 2/2/2018 6:30 PM, Junio C Hamano wrote:

Jonathan Tan  writes:


After reviewing Derrick's Serialized Git Commit Graph patches [1], I
noticed that "[PATCH v2 11/14] commit: integrate commit graph with
commit parsing" contains (in bsearch_graph) a repeat of some packfile
functionality. Here is a pack that refactors that functionality out.


Yay.  I had exactly the same reaction to that part of the series.



Thanks for doing this refactor. I'm a fan of reducing code clones, but 
also don't want to break well-worn code paths.


Jonathan: While you are doing this, I'm guessing you could use your new 
method to replace (and maybe speed up) the binary search in 
sha1_name.c:find_abbrev_len_for_pack(). Otherwise, I can take a stab at 
it next week.


Please add
Reviewed-by: Derrick Stolee 

Thanks,
-Stolee

Re: [PATCH v2 05/14] commit-graph: implement git-commit-graph --write

[Derrick Stolee]

On 2/2/2018 5:48 PM, Junio C Hamano wrote:

Stefan Beller  writes:


It is true for git-submodule and a few others (the minority of commands IIRC)
git-tag for example takes subcommands such as --list or --verify.
https://public-inbox.org/git/xmqqiomodkt9@gitster.dls.corp.google.com/


Thanks.  It refers to an article at gmane, which is not readily
accessible unless you use newsreader.  The original discussion it
refers to appears at:

 https://public-inbox.org/git/7vbo5itjfl@alter.siamese.dyndns.org/

for those who are interested.


Thanks for the links.


I am still not sure if it is a good design to add a new command like
this series does, though.  I would naively have expected that this
would be a new pack index format that is produced by pack-objects
and index-pack, for example, in which case its maintenance would
almost be invisible to end users (i.e. just like how the pack bitmap
feature was added to the system).


I agree that the medium-term goal is to have this happen without user 
intervention. Something like a "core.autoCommitGraph" setting to trigger 
commit-graph writes during other cleanup activities, such as a repack or 
a gc.


I don't think pairing this with pack-objects or index-pack is a good 
direction, because the commit graph is not locked into a packfile the 
way the bitmap is. In fact, the entire ODB could be replaced 
independently and the graph is still valid (the commits in the graph may 
no longer have their paired commits in the ODB due to a GC; you should 
never navigate to those commits without having a ref pointing to them, 
so this is not immediately a problem).


This sort of interaction with GC is one reason why I did not include the 
automatic updates in this patch. The integration with existing 
maintenance tasks will be worth discussion in its own right. I'd rather 
demonstrate the value of having a graph (even if it is currently 
maintained manually) and then follow up with a focus to integrate with 
repack, gc, etc.


I plan to clean up this patch on Monday given the feedback I received 
the last two days (Thanks Jonathan and Szeder!). However, if the current 
builtin design will block merging, then I'll wait until we can find one 
that works.


Thanks,
-Stolee

Re: [PATCH v7 16/31] merge-recursive: split out code for determining diff_filepairs

[Elijah Newren]

On Fri, Feb 2, 2018 at 4:06 PM, Stefan Beller  wrote:
> On Tue, Jan 30, 2018 at 3:25 PM, Elijah Newren  wrote:

>> +   ret = malloc(sizeof(struct diff_queue_struct));
>
> Please use xmalloc() and while at it, please use "*ret" as the argument
> to sizeof. The reason is slightly better maintainability, as then the type
> of ret can be changed at the declaration and the sizeof computation is still
> correct.

Will do.

>> +   ret->queue = diff_queued_diff.queue;
>> +   ret->nr = diff_queued_diff.nr;
>> +   /* Ignore diff_queued_diff.alloc; we won't be changing size at all */
>> +
>> +   opts.output_format = DIFF_FORMAT_NO_OUTPUT;
>> +   diff_queued_diff.nr = 0;
>> +   diff_queued_diff.queue = NULL;
>> +   diff_flush();
>
> The comment is rather meant for the later lines or the former lines
> (where ret is assigned), the empty line seems like it could go before
> the comment?

Perhaps I should just replaced the first three lines, including the
comment, with
  *ret = diff_queued_diff;
?  I was probably thinking along that track, which is why I had the
comment grouped with lines above it, but was for whatever reason just
assigning each value and then noting that one of them was actually
unnecessary.  But it wouldn't hurt to copy either.

Re: [PATCH v7 19/31] merge-recursive: add get_directory_renames()

[Stefan Beller]

On Tue, Jan 30, 2018 at 3:25 PM, Elijah Newren  wrote:
> This populates a list of directory renames for us.  The list of
> directory renames is not yet used, but will be in subsequent commits.
>
> Signed-off-by: Elijah Newren 
> ---
>  merge-recursive.c | 155 
> --
>  1 file changed, 152 insertions(+), 3 deletions(-)
>
> diff --git a/merge-recursive.c b/merge-recursive.c
> index 40ed8e1f39..c75d3a5139 100644
> --- a/merge-recursive.c
> +++ b/merge-recursive.c
> @@ -1393,6 +1393,138 @@ static struct diff_queue_struct *get_diffpairs(struct 
> merge_options *o,
> return ret;
>  }
>
> +static void get_renamed_dir_portion(const char *old_path, const char 
> *new_path,
> +   char **old_dir, char **new_dir)
> +{
> +   char *end_of_old, *end_of_new;
> +   int old_len, new_len;
> +
> +   *old_dir = NULL;
> +   *new_dir = NULL;
> +
> +   /* For

comment style.

/*
 * we prefer to keep the beginning
 * and ending line of a comment free.
 */
/* unless you write single line comments */

> +*"a/b/c/d/foo.c" -> "a/b/something-else/d/foo.c"
> +* the "d/foo.c" part is the same, we just want to know that
> +*"a/b/c" was renamed to "a/b/something-else"
> +* so, for this example, this function returns "a/b/c" in
> +* *old_dir and "a/b/something-else" in *new_dir.

So we would not see multi-directory renames?

"a/b/c/d/foo.c" -> "a/b/something-else/e/foo.c"

could be detected as

"a/b/{c/d/ => something-else/e}/foo.c"

I assume this patch series is not bringing that to the table.
(which is fine, I am just wondering)

> +*
> +* Also, if the basename of the file changed, we don't care.  We
> +* want to know which portion of the directory, if any, changed.
> +*/
> +   end_of_old = strrchr(old_path, '/');
> +   end_of_new = strrchr(new_path, '/');
> +
> +   if (end_of_old == NULL || end_of_new == NULL)
> +   return;

return early as the files are in the top level, and apparently we do
not support top level renaming?

What about a commit like 81b50f3ce4 (Move 'builtin-*' into
a 'builtin/' subdirectory, 2010-02-22) ?

Well that specific commit left many files outside the new builtin dir,
but conceptually we could see a directory rename of

/* => /src/*

> +   while (*--end_of_new == *--end_of_old &&
> +  end_of_old != old_path &&
> +  end_of_new != new_path)
> +   ; /* Do nothing; all in the while loop */

We have to compare manually as we'd want to find
the first non-equal and there doesn't seem to be a good
library function for that.

Assuming many repos are UTF8 (including in their paths),
how does this work with display characters longer than one char?
It should be fine as we cut at the slash?

> +   /*
> +* We've found the first non-matching character in the directory
> +* paths.  That means the current directory we were comparing
> +* represents the rename.  Move end_of_old and end_of_new back
> +* to the full directory name.
> +*/
> +   if (*end_of_old == '/')
> +   end_of_old++;
> +   if (*end_of_old != '/')
> +   end_of_new++;
> +   end_of_old = strchr(end_of_old, '/');
> +   end_of_new = strchr(end_of_new, '/');
> +
> +   /*
> +* It may have been the case that old_path and new_path were the same
> +* directory all along.  Don't claim a rename if they're the same.
> +*/
> +   old_len = end_of_old - old_path;
> +   new_len = end_of_new - new_path;
> +
> +   if (old_len != new_len || strncmp(old_path, new_path, old_len)) {

How often are we going to see this string-is-equal case?
Would it make sense to do that first in the function?

> +   *old_dir = xstrndup(old_path, old_len);
> +   *new_dir = xstrndup(new_path, new_len);
> +   }
> +}
> +
> +static struct hashmap *get_directory_renames(struct diff_queue_struct *pairs,
> +struct tree *tree)
> +{
> +   struct hashmap *dir_renames;
> +   struct hashmap_iter iter;
> +   struct dir_rename_entry *entry;
> +   int i;
> +
> +   dir_renames = malloc(sizeof(struct hashmap));

xmalloc

> +   dir_rename_init(dir_renames);
> +   for (i = 0; i < pairs->nr; ++i) {
> +   struct string_list_item *item;
> +   int *count;
> +   struct diff_filepair *pair = pairs->queue[i];
> +   char *old_dir, *new_dir;
> +
> +   /* File not part of directory rename if it wasn't renamed */
> +   if (pair->status != 'R')
> +   continue;
> +
> +   get_renamed_dir_portion(pair->one->path, pair->two->path,
> +   _dir,_dir);
> +   

Re: [PATCH v7 18/31] merge-recursive: make a helper function for cleanup for handle_renames

[Stefan Beller]

On Tue, Jan 30, 2018 at 3:25 PM, Elijah Newren  wrote:
> In anticipation of more involved cleanup to come, make a helper function
> for doing the cleanup at the end of handle_renames.  Rename the already
> existing cleanup_rename[s]() to final_cleanup_rename[s](), name the new
> helper initial_cleanup_rename(), and leave the big comment in the code
> about why we can't do all the cleanup at once.
>
> Signed-off-by: Elijah Newren 

This mostly touches new code of the series, so it feels a bit unnecessary.
I don't feel strongly about it being a separate patch, as the patches up to
now are easy to review. (and squashing may harm reviewability)

But the code makes sense.

Stefan

Re: [PATCH v7 17/31] merge-recursive: add a new hashmap for storing directory renames

[Stefan Beller]

On Tue, Jan 30, 2018 at 3:25 PM, Elijah Newren  wrote:
> This just adds dir_rename_entry and the associated functions; code using
> these will be added in subsequent commits.
>
> Signed-off-by: Elijah Newren 
> ---
>  merge-recursive.c | 35 +++
>  merge-recursive.h |  8 
>  2 files changed, 43 insertions(+)
>
> diff --git a/merge-recursive.c b/merge-recursive.c
> index 8ac69e1cbb..3b6d0e3f70 100644
> --- a/merge-recursive.c
> +++ b/merge-recursive.c
> @@ -49,6 +49,41 @@ static unsigned int path_hash(const char *path)
> return ignore_case ? strihash(path) : strhash(path);
>  }
>
> +static struct dir_rename_entry *dir_rename_find_entry(struct hashmap 
> *hashmap,
> + char *dir)
> +{
> +   struct dir_rename_entry key;
> +
> +   if (dir == NULL)
> +   return NULL;
> +   hashmap_entry_init(, strhash(dir));
> +   key.dir = dir;
> +   return hashmap_get(hashmap, , NULL);
> +}
> +
> +static int dir_rename_cmp(void *unused_cmp_data,
> + const struct dir_rename_entry *e1,
> + const struct dir_rename_entry *e2,
> + const void *unused_keydata)
> +{
> +   return strcmp(e1->dir, e2->dir);
> +}
> +
> +static void dir_rename_init(struct hashmap *map)
> +{
> +   hashmap_init(map, (hashmap_cmp_fn) dir_rename_cmp, NULL, 0);

See 55c965f3a2 (Merge branch 'sb/hashmap-cleanup', 2017-08-11) or rather
201c14e375 (attr.c: drop hashmap_cmp_fn cast, 2017-06-30) for an attempt to
keep out the casting in the init, but cast the void->type inside the function.

> +static void dir_rename_entry_init(struct dir_rename_entry *entry,
> + char *directory)
> +{
> +   hashmap_entry_init(entry, strhash(directory));
> +   entry->dir = directory;
> +   entry->non_unique_new_dir = 0;
> +   strbuf_init(>new_dir, 0);
> +   string_list_init(>possible_new_dirs, 0);
> +}
> +
>  static void flush_output(struct merge_options *o)
>  {
> if (o->buffer_output < 2 && o->obuf.len) {
> diff --git a/merge-recursive.h b/merge-recursive.h
> index 80d69d1401..d7f4cc80c1 100644
> --- a/merge-recursive.h
> +++ b/merge-recursive.h
> @@ -29,6 +29,14 @@ struct merge_options {
> struct string_list df_conflict_file_set;
>  };
>
> +struct dir_rename_entry {
> +   struct hashmap_entry ent; /* must be the first member! */
> +   char *dir;
> +   unsigned non_unique_new_dir:1;
> +   struct strbuf new_dir;
> +   struct string_list possible_new_dirs;
> +};

Could you add comments what these are and if they have any constraints?
e.g. is 'dir' the full path from the root of the repo and does it end
with a '/' ?

Having a stringlist of potentially new dirs sounds like the algorithm is
at least n^2, but how do I know? I'll read on.

This patch only adds static functions, so some compilers may even refuse
to compile after this patch (-Werror -Wunused). I am not sure how strict we
are there, but as git-bisect still hasn't learned about going only
into the first
parent to have bisect working on a "per series" level rather than a "per commit"
level, it is possible that someone will end up on this commit in the future and
it won't compile well. :/

Not sure what to recommend, maybe squash this with the patch that makes
use of these functions?

Thanks,
Stefan

Re: [PATCH v7 16/31] merge-recursive: split out code for determining diff_filepairs

[Stefan Beller]

On Tue, Jan 30, 2018 at 3:25 PM, Elijah Newren  wrote:

> @@ -1354,10 +1345,43 @@ static struct string_list *get_renames(struct 
> merge_options *o,
> diffcore_std();
> if (opts.needed_rename_limit > o->needed_rename_limit)
> o->needed_rename_limit = opts.needed_rename_limit;
> -   for (i = 0; i < diff_queued_diff.nr; ++i) {
> +
> +   ret = malloc(sizeof(struct diff_queue_struct));

Please use xmalloc() and while at it, please use "*ret" as the argument
to sizeof. The reason is slightly better maintainability, as then the type
of ret can be changed at the declaration and the sizeof computation is still
correct.

> +   ret->queue = diff_queued_diff.queue;
> +   ret->nr = diff_queued_diff.nr;
> +   /* Ignore diff_queued_diff.alloc; we won't be changing size at all */
> +
> +   opts.output_format = DIFF_FORMAT_NO_OUTPUT;
> +   diff_queued_diff.nr = 0;
> +   diff_queued_diff.queue = NULL;
> +   diff_flush();

The comment is rather meant for the later lines or the former lines
(where ret is assigned), the empty line seems like it could go before
the comment?

Re: [PATCH v7 15/31] merge-recursive: make !o->detect_rename codepath more obvious

[Stefan Beller]

On Tue, Jan 30, 2018 at 3:25 PM, Elijah Newren  wrote:
> Previously, if !o->detect_rename then get_renames() would return an
> empty string_list, and then process_renames() would have nothing to
> iterate over.  It seems more straightforward to simply avoid calling
> either function in that case.
>
> Signed-off-by: Elijah Newren 
> ---
>  merge-recursive.c | 11 +--
>  1 file changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/merge-recursive.c b/merge-recursive.c
> index 1986af79a9..4e6d0c248e 100644
> --- a/merge-recursive.c
> +++ b/merge-recursive.c
> @@ -1338,8 +1338,6 @@ static struct string_list *get_renames(struct 
> merge_options *o,
> struct diff_options opts;
>
> renames = xcalloc(1, sizeof(struct string_list));
> -   if (!o->detect_rename)
> -   return renames;
>
> diff_setup();
> opts.flags.recursive = 1;
> @@ -1657,6 +1655,12 @@ static int handle_renames(struct merge_options *o,
>   struct string_list *entries,
>   struct rename_info *ri)
>  {
> +   ri->head_renames = NULL;
> +   ri->merge_renames = NULL;
> +
> +   if (!o->detect_rename)
> +   return 1;
> +
> ri->head_renames  = get_renames(o, head, common, head, merge, 
> entries);
> ri->merge_renames = get_renames(o, merge, common, head, merge, 
> entries);

So this pulls the condition out and we just do not call get_renames.
Makes sense as then we also do not allocate memory for the stringlists
in get_renames

Reviewed-by: Stefan Beller 

Re: [PATCH v7 14/31] merge-recursive: fix leaks of allocated renames and diff_filepairs

[Stefan Beller]

On Tue, Jan 30, 2018 at 3:25 PM, Elijah Newren  wrote:
> 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 

Thanks for spotting the memleaks and fixing them in here.

If this series takes longer to review/cook, this patch
could go in separately? (with the last patch undone :/)

Re: Bug Report: Subtrees and GPG Signed Commits

[Junio C Hamano]

Stephen R Guglielmo  writes:

> On Tue, Jan 30, 2018 at 6:37 PM, Avery Pennarun  wrote:
>>
>> Sorry I can't help more.
>>
>> Good luck,
>>
>> Avery
>
> Thanks all for the discussion/replies.
>
> We use subtrees extensively in our environment right now. The "sub"
> repos (90+) are located on GitHub, while the "main/parent" repo is
> provided by a vendor on website hosting infrastructure.
>
> I will take a look at:
> git/Documentation/CodingGuidelines
> git/Documentation/SubmittingPatches
> git/contrib/subtree/
>
> Should I follow up in this thread with a patch (it might be a while)?

These three are good place to start at.  You may find the output of
"git shortlog --no-merges --since=N.months contrib/subtree" and "git
blame contrib/subtree" also a good source of whom to ask for help.
As we said on this thread, this is a corner of the contrib/ section
that nobody seems to be actively working on, so if you really depend
on it working well, you might have to take the ownership of it ;-)

Thanks.

Re: [PATCH v7 13/31] merge-recursive: introduce new functions to handle rename logic

[Stefan Beller]

On Tue, Jan 30, 2018 at 3:25 PM, Elijah Newren  wrote:
> The amount of logic in merge_trees() relative to renames was just a few
> lines, but split it out into new handle_renames() and cleanup_renames()
> functions to prepare for additional logic to be added to each.  No code or
> logic changes, just a new place to put stuff for when the rename detection
> gains additional checks.
>
> Note that process_renames() records pointers to various information (such
> as diff_filepairs) into rename_conflict_info structs.  Even though the
> rename string_lists are not directly used once handle_renames() completes,
> we should not immediately free the lists at the end of that function
> because they store the information referenced in the rename_conflict_info,
> which is used later in process_entry().  Thus the reason for a separate
> cleanup_renames().
>
> Signed-off-by: Elijah Newren 

Reviewed-by: Stefan Beller 

Re: [PATCH 0/2] Refactor hash search with fanout table

[Junio C Hamano]

Jonathan Tan  writes:

> After reviewing Derrick's Serialized Git Commit Graph patches [1], I
> noticed that "[PATCH v2 11/14] commit: integrate commit graph with
> commit parsing" contains (in bsearch_graph) a repeat of some packfile
> functionality. Here is a pack that refactors that functionality out.

Yay.  I had exactly the same reaction to that part of the series.

Thansk.

Re: [PATCH] rebase: add --allow-empty-message option

[Junio C Hamano]

Genki Sky  writes:

> --> Motivation
>
> commit 4bee95847 ("cherry-pick: add --allow-empty-message option", 2012-08-02)
> started doing this work, but it was never completed. For more discussion
> on why this approach was chosen, see the thread beginning here:
>
>   https://public-inbox.org/git/2012080658.ga21...@arachsys.com/
>
> https://stackoverflow.com/q/8542304 also shows this as a desirable
> feature, and that the workaround is non-trivial to get right.
>
> --> Implementation
>
> Add a new --allow-empty-message flag. Propagate it to all calls of 'git
> commit', 'git cherry-pick', and 'git rebase--helper' within the rebase
> scripts.
>
> Signed-off-by: Genki Sky 
> ---

Do you have our project history so that you can try running "git
log" to realize that the above does not quite match how people write
their log messages?  If not, please obtain one and do so ;-)

 - We discourage log messages from not explaining what *it* needs to
   explain itself and only referring to external resources.  The
   first part of the above is a typical anti-pattern.  The only
   thing readers can gather from "Motivation" part without refering
   to outside resources is it is a moral follow-up of 4bee95847
   whatever "this work" is doing, without being told what approach
   "this approach" means, etc.  URLs are good as supporting info,
   but there must be something they are meant to support readable in
   the log message itself.

 - Also we do not organize our log messages as "-->" bulletted
   chapters.  For this particular commit, once the first part
   becomes self-sufficient, I think it is sufficient to drop these
   bulletted headlines and have two paragraphs (first describing the
   problem being solved, then describing how the patch realizes that
   solution).

I think the changes to the code are sensible.  As Dscho said, I
found the new test script somewhat iffy.  Does it have to be a
completely new test script (as opposed to an additional test or two
to an existing tests for rebase that checks a similar feature like
keep-empty)?  Would it make it simpler to piggy back on an existing
one?

Re: [PATCH v7 12/31] merge-recursive: move the get_renames() function

[Stefan Beller]

On Tue, Jan 30, 2018 at 3:25 PM, Elijah Newren  wrote:
> 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.

git log --grep "I want" --format="%ad %an %s"

Nowadays we write commit messages slightly differently, more
passively, but I let others comment on the wording of the commit
message if they feel inclined to do so. I can fully understand what
this commit is all about.


> +   struct string_list_item *item;
> +   struct rename *re;
> +   struct diff_filepair *pair = diff_queued_diff.queue[i];
> +
> +   if (pair->status != 'R') {

It is not an exact move, you snuck in an empty line
between the variable declarations and this condition. :)
No big deal, actually I like it for readability.
(related 
https://public-inbox.org/git/cagz79kzx2fsejd04zr5-oufu6dlhiohbkxv4u8vewl0oprf...@mail.gmail.com/)

Thanks,
Stefan

Re: Git on Windows maps creation time onto changed time

[Johannes Schindelin]

Hi Keith,

On Fri, 2 Feb 2018, Keith Goldfarb wrote:

> > On Feb 2, 2018, at 1:18 PM, Johannes Schindelin 
> >  wrote:
> > 
> > You cannot assume that the inode numbers are identical between file
> > systems/operating systems. That's simply not going to work.
> 
> Yes, I agree with you, I cannot assume this, so I checked. In my case, the 
> inode numbers are indeed identical.
> 
> > I know you really want *so hard* for the same working directory to be
> > accessible from both Windows and Linux. I have a lot of sympathy for that
> > sentiment. Though I do not see much chance for success on that front.
> 
> I’m certainly willing to accept that there are going to be limitations by 
> using a filesystem from two different operating systems. But regardless of 
> the problems caused by that pattern, would you agree that the Windows code 
> should be using the actual inode number (well, 32-bits of it) instead of zero?

In all likelihood, you have enabled the FS Cache feature. And for a good
reason: it makes many Git operations much, much faster.

Still not as fast as on Linux because Git is quite tuned to assume all the
Linux semantics. So instead of using tell-tales for file modifications
that are portable, Git uses tell-tales that are readily available on
Linux. And instead of using access patterns that are portable, Git assumes
POSIX semantics where you simply use opendir()/readdir()/closedir() and in
that inner loop, you call lstat() to get all the stat data.

This is obviously not portable beyond POSIX. On Windows, for example,
there are really fast FindFirstFile()/FindNextFile() APIs that even allow
you to specify patterns to match (which Linux has to do manually, by
inspecting the file name obtained via readdir()). You even get some stat
data back at the same time, really fast, without additional API calls,
unlike on Linux.

But Git's access patterns really are tuned to the *dir() way, and we have
to emulate it in Git for Windows. This is very, very slow.

The FS Cache feature tries to gain back some speed by using
FindFirstFile()/FindNextFile() pre-emptively and caching the data, looking
it up upon the next time stat data is queried.

The stat data obtained that way does not, however, include Change Time nor
inode number.

And that, dear children, was the story for the day: this is why Git for
Windows uses the CreationTime instead of the Change Time and why it marks
inode as 0; Because to do it accurately would be a lot slower, and we
cannot afford even slower because we are already super slow because we
have to work around Git's assumptions as to what APIs are available.

Ciao,
Johannes

F.LLI PISTOLESI Snc

[. F.LLI PISTOLESI Snc]

Hello ,

I am looking for a reliable supplier /manufacturer of products for sell in 
Europe.

I came across your listing and wanted to get some information regarding minimum 
Order Quantities, FOB pricing and also the possibility of packaging including 
payments terms.

So could you please get back to be with the above informations as soon as 
possible .

My email is :tm6428...@gmail.com

Many thanks and i looking forward to hearing from you and hopefully placing an 
order with you company.

Best Regards
Lorenzo Delleani.

F.LLI PISTOLESI Snc Company P.O. box 205
2740 AE Waddinxveen
The Netherlands

Re: [PATCH v2 05/14] commit-graph: implement git-commit-graph --write

[Junio C Hamano]

Stefan Beller  writes:

> It is true for git-submodule and a few others (the minority of commands IIRC)
> git-tag for example takes subcommands such as --list or --verify.
> https://public-inbox.org/git/xmqqiomodkt9@gitster.dls.corp.google.com/

Thanks.  It refers to an article at gmane, which is not readily
accessible unless you use newsreader.  The original discussion it
refers to appears at:

https://public-inbox.org/git/7vbo5itjfl@alter.siamese.dyndns.org/

for those who are interested.

I am still not sure if it is a good design to add a new command like
this series does, though.  I would naively have expected that this
would be a new pack index format that is produced by pack-objects
and index-pack, for example, in which case its maintenance would
almost be invisible to end users (i.e. just like how the pack bitmap
feature was added to the system).

Re: [PATCH v2 10/27] protocol: introduce enum protocol_version value protocol_v2

[Brandon Williams]

On 01/31, Derrick Stolee wrote:
> On 1/25/2018 6:58 PM, Brandon Williams wrote:
> > Introduce protocol_v2, a new value for 'enum protocol_version'.
> > Subsequent patches will fill in the implementation of protocol_v2.
> > 
> > Signed-off-by: Brandon Williams 
> > ---
> >   builtin/fetch-pack.c   | 3 +++
> >   builtin/receive-pack.c | 6 ++
> >   builtin/send-pack.c| 3 +++
> >   builtin/upload-pack.c  | 7 +++
> >   connect.c  | 3 +++
> >   protocol.c | 2 ++
> >   protocol.h | 1 +
> >   remote-curl.c  | 3 +++
> >   transport.c| 9 +
> >   9 files changed, 37 insertions(+)
> > 
> > diff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c
> > index 85d4faf76..f492e8abd 100644
> > --- a/builtin/fetch-pack.c
> > +++ b/builtin/fetch-pack.c
> > @@ -201,6 +201,9 @@ int cmd_fetch_pack(int argc, const char **argv, const 
> > char *prefix)
> >PACKET_READ_GENTLE_ON_EOF);
> > switch (discover_version()) {
> > +   case protocol_v2:
> > +   die("support for protocol v2 not implemented yet");
> > +   break;
> > case protocol_v1:
> > case protocol_v0:
> > get_remote_heads(, , 0, NULL, );
> > diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
> > index b7ce7c7f5..3656e94fd 100644
> > --- a/builtin/receive-pack.c
> > +++ b/builtin/receive-pack.c
> > @@ -1963,6 +1963,12 @@ int cmd_receive_pack(int argc, const char **argv, 
> > const char *prefix)
> > unpack_limit = receive_unpack_limit;
> > switch (determine_protocol_version_server()) {
> > +   case protocol_v2:
> > +   /*
> > +* push support for protocol v2 has not been implemented yet,
> > +* so ignore the request to use v2 and fallback to using v0.
> > +*/
> > +   break;
> > case protocol_v1:
> > /*
> >  * v1 is just the original protocol with a version string,
> > diff --git a/builtin/send-pack.c b/builtin/send-pack.c
> > index 83cb125a6..b5427f75e 100644
> > --- a/builtin/send-pack.c
> > +++ b/builtin/send-pack.c
> > @@ -263,6 +263,9 @@ int cmd_send_pack(int argc, const char **argv, const 
> > char *prefix)
> >PACKET_READ_GENTLE_ON_EOF);
> > switch (discover_version()) {
> > +   case protocol_v2:
> > +   die("support for protocol v2 not implemented yet");
> > +   break;
> > case protocol_v1:
> > case protocol_v0:
> > get_remote_heads(, _refs, REF_NORMAL,
> > diff --git a/builtin/upload-pack.c b/builtin/upload-pack.c
> > index 2cb5cb35b..8d53e9794 100644
> > --- a/builtin/upload-pack.c
> > +++ b/builtin/upload-pack.c
> > @@ -47,6 +47,13 @@ int cmd_upload_pack(int argc, const char **argv, const 
> > char *prefix)
> > die("'%s' does not appear to be a git repository", dir);
> > switch (determine_protocol_version_server()) {
> > +   case protocol_v2:
> > +   /*
> > +* fetch support for protocol v2 has not been implemented yet,
> > +* so ignore the request to use v2 and fallback to using v0.
> > +*/
> > +   upload_pack();
> > +   break;
> > case protocol_v1:
> > /*
> >  * v1 is just the original protocol with a version string,
> > diff --git a/connect.c b/connect.c
> > index db3c9d24c..f2157a821 100644
> > --- a/connect.c
> > +++ b/connect.c
> > @@ -84,6 +84,9 @@ enum protocol_version discover_version(struct 
> > packet_reader *reader)
> > /* Maybe process capabilities here, at least for v2 */
> > switch (version) {
> > +   case protocol_v2:
> > +   die("support for protocol v2 not implemented yet");
> > +   break;
> > case protocol_v1:
> > /* Read the peeked version line */
> > packet_reader_read(reader);
> > diff --git a/protocol.c b/protocol.c
> > index 43012b7eb..5e636785d 100644
> > --- a/protocol.c
> > +++ b/protocol.c
> > @@ -8,6 +8,8 @@ static enum protocol_version parse_protocol_version(const 
> > char *value)
> > return protocol_v0;
> > else if (!strcmp(value, "1"))
> > return protocol_v1;
> > +   else if (!strcmp(value, "2"))
> > +   return protocol_v2;
> > else
> > return protocol_unknown_version;
> >   }
> > diff --git a/protocol.h b/protocol.h
> > index 1b2bc94a8..2ad35e433 100644
> > --- a/protocol.h
> > +++ b/protocol.h
> > @@ -5,6 +5,7 @@ enum protocol_version {
> > protocol_unknown_version = -1,
> > protocol_v0 = 0,
> > protocol_v1 = 1,
> > +   protocol_v2 = 2,
> >   };
> >   /*
> > diff --git a/remote-curl.c b/remote-curl.c
> > index 9f6d07683..dae8a4a48 100644
> > --- a/remote-curl.c
> > +++ b/remote-curl.c
> > @@ -185,6 +185,9 @@ static struct ref *parse_git_refs(struct discovery 
> > *heads, int for_push)
> >PACKET_READ_GENTLE_ON_EOF);
> > switch (discover_version()) {
> > +   case 

Re: Git on Windows maps creation time onto changed time

[Keith Goldfarb]

> On Feb 2, 2018, at 1:18 PM, Johannes Schindelin  
> wrote:
> 
> You cannot assume that the inode numbers are identical between file
> systems/operating systems. That's simply not going to work.

Yes, I agree with you, I cannot assume this, so I checked. In my case, the 
inode numbers are indeed identical.

> I know you really want *so hard* for the same working directory to be
> accessible from both Windows and Linux. I have a lot of sympathy for that
> sentiment. Though I do not see much chance for success on that front.

I’m certainly willing to accept that there are going to be limitations by using 
a filesystem from two different operating systems. But regardless of the 
problems caused by that pattern, would you agree that the Windows code should 
be using the actual inode number (well, 32-bits of it) instead of zero?

K.

[PATCH 2/2] packfile: refactor hash search with fanout table

[Jonathan Tan]

Subsequent patches will introduce file formats that make use of a fanout
array and a sorted table containing hashes, just like packfiles.
Refactor the hash search in packfile.c into its own function, so that
those patches can make use of it as well.

Signed-off-by: Jonathan Tan 
---
 packfile.c| 19 +--
 sha1-lookup.c | 24 
 sha1-lookup.h | 21 +
 3 files changed, 50 insertions(+), 14 deletions(-)

diff --git a/packfile.c b/packfile.c
index 58bdced3b..29f5dc239 100644
--- a/packfile.c
+++ b/packfile.c
@@ -1712,7 +1712,8 @@ off_t find_pack_entry_one(const unsigned char *sha1,
 {
const uint32_t *level1_ofs = p->index_data;
const unsigned char *index = p->index_data;
-   unsigned hi, lo, stride;
+   unsigned stride;
+   int ret;
 
if (!index) {
if (open_pack_index(p))
@@ -1725,8 +1726,6 @@ off_t find_pack_entry_one(const unsigned char *sha1,
index += 8;
}
index += 4 * 256;
-   hi = ntohl(level1_ofs[*sha1]);
-   lo = ((*sha1 == 0x0) ? 0 : ntohl(level1_ofs[*sha1 - 1]));
if (p->index_version > 1) {
stride = 20;
} else {
@@ -1734,17 +1733,9 @@ off_t find_pack_entry_one(const unsigned char *sha1,
index += 4;
}
 
-   while (lo < hi) {
-   unsigned mi = lo + (hi - lo) / 2;
-   int cmp = hashcmp(index + mi * stride, sha1);
-
-   if (!cmp)
-   return nth_packed_object_offset(p, mi);
-   if (cmp > 0)
-   hi = mi;
-   else
-   lo = mi+1;
-   }
+   ret = bsearch_hash(sha1, level1_ofs, index, stride);
+   if (ret >= 0)
+   return nth_packed_object_offset(p, ret);
return 0;
 }
 
diff --git a/sha1-lookup.c b/sha1-lookup.c
index 4cf3ebd92..d11c5e526 100644
--- a/sha1-lookup.c
+++ b/sha1-lookup.c
@@ -99,3 +99,27 @@ int sha1_pos(const unsigned char *sha1, void *table, size_t 
nr,
} while (lo < hi);
return -lo-1;
 }
+
+int bsearch_hash(const unsigned char *sha1, const void *fanout_,
+const void *table_, size_t stride)
+{
+   const uint32_t *fanout = fanout_;
+   const unsigned char *table = table_;
+   int hi, lo;
+
+   hi = ntohl(fanout[*sha1]);
+   lo = ((*sha1 == 0x0) ? 0 : ntohl(fanout[*sha1 - 1]));
+
+   while (lo < hi) {
+   unsigned mi = lo + (hi - lo) / 2;
+   int cmp = hashcmp(table + mi * stride, sha1);
+
+   if (!cmp)
+   return mi;
+   if (cmp > 0)
+   hi = mi;
+   else
+   lo = mi + 1;
+   }
+   return -lo - 1;
+}
diff --git a/sha1-lookup.h b/sha1-lookup.h
index cf5314f40..3c59e9cb1 100644
--- a/sha1-lookup.h
+++ b/sha1-lookup.h
@@ -7,4 +7,25 @@ extern int sha1_pos(const unsigned char *sha1,
void *table,
size_t nr,
sha1_access_fn fn);
+
+/*
+ * Searches for sha1 in table, using the given fanout table to determine the
+ * interval to search, then using binary search. Returns the element index of
+ * the position found if successful, -i-1 if not (where i is the index of the
+ * least element that is greater than sha1).
+ *
+ * Takes the following parameters:
+ *
+ *  - sha1: the hash to search for
+ *  - fanout: a 256-element array of NETWORK-order 32-bit integers; the integer
+ *at position i represents the number of elements in table whose first byte
+ *is less than or equal to i
+ *  - table: a sorted list of hashes with optional extra information in between
+ *  - stride: distance between two consecutive elements in table (should be
+ *GIT_MAX_RAWSZ or greater)
+ *
+ * This function does not verify the validity of the fanout table.
+ */
+extern int bsearch_hash(const unsigned char *sha1, const void *fanout,
+   const void *table, size_t stride);
 #endif
-- 
2.16.0.rc1.238.g530d649a79-goog

[PATCH 1/2] packfile: remove GIT_DEBUG_LOOKUP log statements

[Jonathan Tan]

In commit 628522ec1439 ("sha1-lookup: more memory efficient search in
sorted list of SHA-1", 2008-04-09), a different algorithm for searching
a sorted list was introduced, together with a set of log statements
guarded by GIT_DEBUG_LOOKUP that are invoked both when using that
algorithm and when using the existing binary search. Those log
statements was meant for experiments and debugging, but with the removal
of the aforementioned different algorithm in commit f1068efefe6d
("sha1_file: drop experimental GIT_USE_LOOKUP search", 2017-08-09),
those log statements are probably no longer necessary.

Remove those statements.

Signed-off-by: Jonathan Tan 
---
 packfile.c | 11 ---
 1 file changed, 11 deletions(-)

diff --git a/packfile.c b/packfile.c
index 4a5fe7ab1..58bdced3b 100644
--- a/packfile.c
+++ b/packfile.c
@@ -1713,10 +1713,6 @@ off_t find_pack_entry_one(const unsigned char *sha1,
const uint32_t *level1_ofs = p->index_data;
const unsigned char *index = p->index_data;
unsigned hi, lo, stride;
-   static int debug_lookup = -1;
-
-   if (debug_lookup < 0)
-   debug_lookup = !!getenv("GIT_DEBUG_LOOKUP");
 
if (!index) {
if (open_pack_index(p))
@@ -1738,17 +1734,10 @@ off_t find_pack_entry_one(const unsigned char *sha1,
index += 4;
}
 
-   if (debug_lookup)
-   printf("%02x%02x%02x... lo %u hi %u nr %"PRIu32"\n",
-  sha1[0], sha1[1], sha1[2], lo, hi, p->num_objects);
-
while (lo < hi) {
unsigned mi = lo + (hi - lo) / 2;
int cmp = hashcmp(index + mi * stride, sha1);
 
-   if (debug_lookup)
-   printf("lo %u hi %u rg %u mi %u\n",
-  lo, hi, hi - lo, mi);
if (!cmp)
return nth_packed_object_offset(p, mi);
if (cmp > 0)
-- 
2.16.0.rc1.238.g530d649a79-goog

[PATCH 0/2] Refactor hash search with fanout table

[Jonathan Tan]

After reviewing Derrick's Serialized Git Commit Graph patches [1], I
noticed that "[PATCH v2 11/14] commit: integrate commit graph with
commit parsing" contains (in bsearch_graph) a repeat of some packfile
functionality. Here is a pack that refactors that functionality out.

Derrick, consider incorporating these patches in your next reroll.

[1] 
https://public-inbox.org/git/1517348383-112294-1-git-send-email-dsto...@microsoft.com/

Jonathan Tan (2):
  packfile: remove GIT_DEBUG_LOOKUP log statements
  packfile: refactor hash search with fanout table

 packfile.c| 30 +-
 sha1-lookup.c | 24 
 sha1-lookup.h | 21 +
 3 files changed, 50 insertions(+), 25 deletions(-)

-- 
2.16.0.rc1.238.g530d649a79-goog

Re: [PATCH v2 13/27] ls-refs: introduce ls-refs server command

[Brandon Williams]

On 01/26, Stefan Beller wrote:
> On Thu, Jan 25, 2018 at 3:58 PM, Brandon Williams  wrote:
> 
> > +ls-refs takes in the following parameters wrapped in packet-lines:
> > +
> > +symrefs
> > +   In addition to the object pointed by it, show the underlying ref
> > +   pointed by it when showing a symbolic ref.
> > +peel
> > +   Show peeled tags.
> 
> Would it make sense to default these two to on, and rather have
> optional no-symrefs and no-peel ?
> 
> That would save bandwidth in the default case, I would think.

Maybe?  That would save sending those strings for each request

-- 
Brandon Williams

Re: [ANNOUNCE] Git for Windows 2.16.1(2)

[Johannes Schindelin]

Hi all, in particular packagers,

On Fri, 2 Feb 2018, Johannes Schindelin wrote:

> It is my pleasure to announce that Git for Windows 2.16.1(2) is
> available from:
> 
> https://git-for-windows.github.io/

Typically, every Git for Windows version is accompanied with updates to
the Pacman repository, so that the packagers can use a Git for Windows SDK
to bundle their updated packages.

In Git for Windows, due to Sebastian Schuberth's prodding not to abuse
GitHub to serve essentially static files, we use BinTray instead.

Sadly, BinTray "rate-limited" Git for Windows' account "due to overuse".
That means that there is currently no convenient update via `pacman -Syu`.

There have been glitches in the past, where uploads "succeeded" but the
files still had not been available for download, and other uploads that
did not succeed because the BinTray service was down even if their status
page claimed "all green".

Obviously, I contacted their support immediately, as I had done in the
past. This time I got a response that I should expect delays because I am
not a customer with a support contract.

Needless to say, I am very disappointed with BinTray at the moment. I
spent a substantial amount of time to automate the uploads, and I hate the
idea that all of that was for naught because we might have to switch to a
more reliable hoster.

In the meantime, I will upload the respective Pacman packages to
https://github.com/git-for-windows/git/releases/tag/v2.16.1.windows.2

Ciao,
Johannes

CONTACT DHL OFFICE IMMEDIATELY FOR DELIVERY OF YOUR ATM MASTERCARD

[Rev:Tony HARRIES]

Attention; Beneficiary,

This is to official inform you that we have been having meetings for the past 
three (3) weeks which ended two days ago with MR. JIM YONG KIM the Former world 
bank president and other seven continent presidents on the congress we treated 
on solution to scam victim problems.

Note: we have decided to contact you following the reports we received from 
anti-fraud international monitoring group your name/email has been submitted to 
us therefore the united nations have agreed to compensate you with the sum of 
(USD$1.5 Million) this compensation is also including international business 
that failed you in past due to government problems etc.

We have arranged your payment through our ATM MasterCard and deposited it in 
DHL Office to deliver it to you which is the latest instruction from the Former 
World Bank president MR. JIM YONG KIM, For your information’s, the delivery 
charges already paid by U.N treasury, the only money you will send to DHL 
office Benin Republic is $165 dollars for security keeping fee, U.N coordinator 
already paid for others charges fees for delivery except the security keeping 
fee, the director of DHL refused to collect the security keeping fee from U.N 
treasury because Direct of DHL office said they don’t know exactly time you 
will contact them to reconfirm your details to avoid counting demur rage.

Therefore be advice to contact DHL Office agent Benin. Rev: Tony harries who is 
in position to post your ATM MasterCard, contact DHL Office  immediately with 
the bellow email & phone number  as listed below.

Contact name: Rev:Tony  HARRIES
Email:(post.dhlbenin.serv...@outlook.com )
Tell: +229-98787436

Make sure you reconfirmed DHL Office your details ASAP as stated below to avoid 
wrong delivery.

Your full name..
Home address:.
Your country...
Your city..
Telephone..
Occupation:...
Age:……..


Let us know as soon as possible you receive your ATM MasterCard for proper 
verifications.

Regards,
MR Paul Ogie.
DEPUTY SECRETARY-GENERAL (U.N)
Benin Republic.

[ANNOUNCE] Git for Windows 2.16.1(2)

[Johannes Schindelin]

Dear Git users,

It is my pleasure to announce that Git for Windows 2.16.1(2) is available from:

https://git-for-windows.github.io/

The main purpose of this out-of-band release is to update the BusyBox, cURL,
Perl and Heimdal components to newer versions.

Changes since Git for Windows v2.16.1 (January 22nd 2018)

New Features

  * Comes with Heimdal v7.5.0.
  * Comes with cURL v7.58.0.
  * Comes with Perl v5.26.1.
  * When using GNU nano as Git's default editor, it is now colorful
(shows syntax-highlighting).
  * Comes with tig v2.3.3.
  * When using Secure Channel as HTTPS transport behind a proxy, it may
be necessary to disable revocation checks, which is now possible.
  * Comes with BusyBox v1.28.0pre.16550.0b3cdd76c.

Bug Fixes

  * When Git spawns processes, now only the necessary file handles are
inherited from the parent process, possibly preventing file locking
issues.
  * The git update command has been renamed to git
update-git-for-windows to avoid confusion where users may think
that git update updates their local repository or worktree.

Filename | SHA-256
 | ---
Git-2.16.1.2-64-bit.exe | 
033939d276775edb17233c7f52cc5d53dcf23351eb9c5ebe0d95f13805ac6aa1
Git-2.16.1.2-32-bit.exe | 
564c8c71da3d0888e8fb153556c1e747b2fe4f4ec94d38963ae7766f61fb5cfd
PortableGit-2.16.1.2-64-bit.7z.exe | 
82009aa70ede01da69cd2ea4ad297aeb1f58c7dd8c1d85425d5246ebdf20
PortableGit-2.16.1.2-32-bit.7z.exe | 
5886321e9d6c619c67116b23c6428afc83f2e63f4eee60f6cafa5b37364ebf04
MinGit-2.16.1.2-64-bit.zip | 
172f4ac31280895867e8ffa35f31f4aa590eef115de4cefcedef5f39cecd75b6
MinGit-2.16.1.2-32-bit.zip | 
17657f7e8c562a299cc0d854e43f348a4d7721ec6e16e24d12f0366d9a7e00fc
MinGit-2.16.1.2-busybox-64-bit.zip | 
58609edfd1ea0135c9247b89ad9fafd8270caa307a5f8d8fdccaf0456b8c8dd0
MinGit-2.16.1.2-busybox-32-bit.zip | 
c7721df06fee403b340ff8083c2334dd450b929341645364c6807fdca6c6e74b
Git-2.16.1.2-64-bit.tar.bz2 | 
949271e375c90f1c4a15d2cc19717d5c01cf58b66f1a06bb3de7c5f19ebba5c7
Git-2.16.1.2-32-bit.tar.bz2 | 
41940a0c5eff47612b891241aa2f2edb82e8ed5d32a7f4bde79e720fcd83a960

Ciao,
Johannes

Re: Git on Windows maps creation time onto changed time

[Johannes Schindelin]

Hi Keith,

On Fri, 2 Feb 2018, Keith Goldfarb wrote:

> > The purpose of these values is to allow to notice a change on the file
> > system without going through the actual file data. Duplicating
> > st_mtime would be pointless.
> 
> Well, I agree with the first statement. I disagree with the 2nd. It’s
> not pointless to fix a problem, and in theory the creation date of a
> file can never change.
> 
> > Don't do that then. Use core.trustctime.
> 
> I am. Unfortunately, my problem isn’t solved by that alone. Perhaps this
> deserves its own thread, but on Windows the st_ino field is set to zero.
> This can also trigger a false positive, causing the whole cache to be
> rebuilt unnecessarily.

You cannot assume that the inode numbers are identical between file
systems/operating systems. That's simply not going to work.

I know you really want *so hard* for the same working directory to be
accessible from both Windows and Linux. I have a lot of sympathy for that
sentiment. Though I do not see much chance for success on that front.

Ciao,
Johannes

Re: [PATCH] rebase: add --allow-empty-message option

[Johannes Schindelin]

Hi,

On Fri, 2 Feb 2018, Genki Sky wrote:

> --> Motivation

I won't comment on the motivation (leaving that up to the Git maintainer),
but on the patch.

The patch on the shell scripts and the C code looks straight-forward
enough, if a little repetitive (but that is hardly your fault).

> diff --git a/t/t3430-rebase-empty-message.sh b/t/t3430-rebase-empty-message.sh
> new file mode 100755
> index 0..74af73f3c
> --- /dev/null
> +++ b/t/t3430-rebase-empty-message.sh
> @@ -0,0 +1,79 @@
> +#!/bin/sh
> +
> +test_description='rebase: option to allow empty commit messages'
> +
> +. ./test-lib.sh
> +. "$TEST_DIRECTORY"/lib-rebase.sh
> +
> +test_expect_success 'setup: three regular commits' '
> + test_commit A &&
> + test_commit B &&
> + test_commit C
> +'

None of these commits have empty commit messages, which is a little
curious for a 'setup' test case.

> +test_expect_success 'rebase -i "reword" should fail to create an empty 
> commit message' '
> + set_fake_editor &&
> + test_must_fail env FAKE_COMMIT_MESSAGE=" " FAKE_LINES="1 reword 2" \
> + git rebase -i HEAD~2
> +'

Why not make this more focused via

... FAKE_LINES="reword 1" git rebase -i HEAD^

The effect will be the same because the first pick will be skipped as an
unnecessary pick anyway.

> +test_expect_success '... but should succeed with --allow-empty-message' '
> + git rebase --abort &&

This should be part of the previous test case:

test_when_finished "test_might_fail git rebase --abort" &&

Also, I think this test case should be folded into the previous test case
(which would make that test_when_finished suggestion moot).

> + set_fake_editor &&
> + FAKE_COMMIT_MESSAGE=" " FAKE_LINES="1 reword 2" \
> + git rebase -i --allow-empty-message HEAD~2
> +'
> +
> +test_expect_success 'rebase -i "fixup" should fail to fixup an empty commit 
> message' '
> + test_commit D &&
> + set_fake_editor &&
> + test_must_fail env FAKE_LINES="1 fixup 2" git rebase -i HEAD~2
> +'
> +
> +test_expect_success '... but should succeed with --allow-empty-message' '
> + git rebase --abort &&
> + FAKE_LINES="1 fixup 2" git rebase -i --allow-empty-message HEAD~2
> +'
> +
> +# The test expects the following rebase to fail. It will only fail if it
> +# actually has to cmd_commit() a new [empty message] commit. However, this
> +# rebase makes no actual changes.

Don't you want to use `--force-rebase` here?

> So if the date does not change in time, it is
> +# possible for it to simply take the original commits exactly as they are.
> +# So, we test_tick() just to be safe.
> +test_expect_success 'rebase --root should fail on an empty commit message' '
> + test_tick &&
> + test_must_fail git rebase --root
> +'
> +
> +test_expect_success '... but should succeed with --allow-empty-message' '
> + git rebase --abort &&
> + git rebase --root --allow-empty-message
> +'
> +
> +test_expect_success 'setup: multiple branches' '
> + git checkout -b branch-keep-empty HEAD^1 &&
> + echo E >E &&
> + git add E &&
> + git commit --allow-empty-message -m "" &&
> + git branch branch-merge
> +'
> +
> +test_expect_success 'rebase --keep-empty should fail on an empty commit 
> message' '
> + test_must_fail git rebase --keep-empty master branch-keep-empty
> +'
> +
> +test_expect_success '... but should succeed with --allow-empty-message' '
> + git cherry-pick --abort &&
> + git rebase --keep-empty --allow-empty-message master branch-keep-empty
> +'

I do not really see why we have to test --keep-empty here. The code paths
overlap with what was tested previously.

> +test_expect_success 'rebase -m should fail on an empty commit message' '
> + test_must_fail git rebase -m master branch-merge
> +'
> +
> +test_expect_success '... but should succeed with --allow-empty-message' '
> + git rebase --abort &&
> + git rebase -m --allow-empty-message master branch-merge
> +'
> +
> +test_done

In general, I would much rather fold the test cases that verify the
behavior with --allow-empty-message into the same test case as verifying
the behavior without --allow-empty-message.

One of my aims in the Git project is to avoid test bloat. I know that
several other active contributors could not care less, and even the Git
maintainer seems to be oblivious to the danger of making a test suite so
unsuitable (both in terms of run-time and in terms of being able to
understand regressions) as to make it less useful. I certainly had painful
discussions on this very mailing list about that, and I still don't feel
heard nor understood.

While your patch certainly is clear enough to make it really easy to
understand regressions, I find that it errs on the side of over-testing.

And this is not an academic consideration. The test suite takes an insane
70-90 minutes *on a fast* Windows machine, even skipping all of the
git-svn tests. That's insane. (And the fact that Git is 

Re: [RFC/PATCH] reset --hard: make use of the pretty machinery

[Junio C Hamano]

Thomas Gummerer  writes:

> In addition to the easier to follow code, this makes the output more
> consistent with other commands that print the title of the commit, such
> as 'git commit --oneline' or 'git checkout', which both use
> 'pp_commit_easy()' with the CMIT_FMT_ONELINE modifier.

Yes, this is absolutely the right thing to do.

> +
> + pp_commit_easy(CMIT_FMT_ONELINE, commit, );
> + if (buf.len > 0)
> + printf(" %s", buf.buf);
> + putchar('\n');
> + strbuf_release();
>  }
>  
>  static void update_index_from_diff(struct diff_queue_struct *q,

Thanks, will queue.

Re: [PATCH v3 0/2] diff: add --stat-with-summary (was --compact-summary)

[Junio C Hamano]

Nguyễn Thái Ngọc Duy   writes:

> Changes since v2 [1]:
>
> - goes back to my original version (yay!) where the extra info
>   is appended after the path name. More is described in 2/2
> - --compact-summary is now renamed --stat-with-summary and implies
>   --stat
> - 1/2 is just a cleanup patch to make it easier to add 2/2

It may be just me and other old timers, but --X-with-Y naming means
quite different thing around these commands, and --stat-with-summary
would hint, at least to us, that it would behave as if the two
options "--stat --summary" are given at the same time.

And from that point of view, the new name is a bit confusing one.

The patches themselves look excellent.  Thanks.

Re: [PATCH v2 00/12] object_id part 11 (the_hash_algo)

[Junio C Hamano]

"brian m. carlson"  writes:

> This series includes various changes to adopt the use of the_hash_algo
> for abstracting hash algorithms away.
>
> Changes from v1:
> * Fix comments referring to SHA-1.
> * Switch hash function wrappers to take git_hash_ctx *.
> * Use a const int variable for a constant value.
> * Wrap an overly long line.

Thanks for working on this.  All changes looked sensible (even
though I spotted one nit in the original, which was moved as-is,
which does not count as a "change" ;-)).

Re: [PATCH v2 01/12] hash: move SHA-1 macros to hash.h

[Junio C Hamano]

"brian m. carlson"  writes:

> +#ifndef platform_SHA_CTX
> +/*
> + * platform's underlying implementation of SHA-1; could be OpenSSL,
> + * blk_SHA, Apple CommonCrypto, etc...  Note that including
> + * SHA1_HEADER may have already defined platform_SHA_CTX for our
> + * own implementations like block-sha1 and ppc-sha1, so we list
> + * the default for OpenSSL compatible SHA-1 implementations here.
> + */

This nit has been with us for a long time, but "Note that including
SHA1_HEADER may have..." has been way too stale a comment for quite
some time.  It was made different from "including SHA1_HEADER" by
f18f816c ("hash.h: move SHA-1 implementation selection into a header
file", 2017-03-11).

Perhaps

Note that we may have already defined platform_SHA_CTX for our
own implementations like block-sha1 and ppc-sha1, so we list
...

or something.

Re: What's cooking in git.git (Jan 2018, #04; Wed, 31)

[Ævar Arnfjörð Bjarmason]

On Fri, Feb 02 2018, Junio C. Hamano jotted:

> Ævar Arnfjörð Bjarmason  writes:
>
>> On Thu, Feb 01 2018, Junio C. Hamano jotted:
>>
>>> * nd/fix-untracked-cache-invalidation (2018-01-24) 5 commits
>>>  - dir.c: stop ignoring opendir() error in open_cached_dir()
>>>  - update-index doc: note a fixed bug in the untracked cache
>>>  - dir.c: fix missing dir invalidation in untracked code
>>>  - dir.c: avoid stat() in valid_cached_dir()
>>>  - status: add a failing test showing a core.untrackedCache bug
>>>
>>>  Some bugs around "untracked cache" feature have been fixed.
>>>
>>>  Will merge to 'next'.
>>
>> The "update-index doc: note a fixed bug in the untracked cache" needs to
>> be amended so it doesn't say "Before 2.16, ":
>
> True; we could just say "earlier", but I am inclined to suggest that
> we get drop it altogether.  Description of historical bugs is of no
> interest with the version that already fixes them, so the _only_
> value the doc update adds is to tell readers that the untracked
> cache feature is still not well proven, and core.untrackedCache may
> serve as an escape hatch from its bugs by disabling the mechanism
> added for the feature.  I am *not* opposed to a replacement of the
> patch that just says something like "This feature has been cause of
> bugs even in recent versions of Git, and you may want to disable it
> as a workaround when you are hit by an otherwise undiscovered bug in
> this area", though.

 - It's my experience that most users today who aren't *nix graybeards
   don't use the documentation shipped on their system as their primary
   source for docs.

   They go to Google and might find the manpage there. Thus this
   documentation will be read by users on pre-2.17 (or whenever this bug
   fix gets included).

 - This is very useful information if you're deploying
   core.untrackedCache across a site with differing git versions. Just
   because you have 2.17 doesn't mean everywhere you're about to deploy
   core.untrackedCache does.

 - In general I agree that we shouldn't be documenting old bugs, but I
   think in this case it makes sense since the bug's really bad. Without
   thinking to disable core.untrackedCache there's seemingly no way to
   fix it without wiping away the index, which might lose you work.

Re: [PATCH/RFC v5 7/7] Careful with CRLF when using e.g. UTF-16 for working-tree-encoding

[Junio C Hamano]

Torsten Bögershausen  writes:

> There are 2 opposite opionions/user expectations here:
>
> a) They are binary in the working tree, so git should leave the line endings
>as is. (Unless specified otherwise in the .attributes file)
> ...
> b) They are text files in the index. Git will convert line endings
>if core.autocrlf is true (or the .gitattributes file specifies "-text")

I sense that you seem to be focusing on the distinction between "in
the working tree" vs "in the index" while contrasting.  The "binary
vs text" in your "binary in wt, text in index" is based on the
default heuristics without any input from end-users or the project
that uses Git that happens to contain such files.  If the users and
the project that uses Git want to treat contents in a path as text,
it is text even when it is (re-)encoded to UTF-16, no?

Such files may be (mis)classified as binary with the default
heuristics when there is no help from what is written in the
.gitattributes file, but here we are talking about the case where
the user explicitly tells us it is in UTF-16, right?  Is there such a
thing as UTF-16 binary?

Re: [PATCHv2] tag: add --edit option

[Eric Sunshine]

On Fri, Feb 2, 2018 at 11:48 AM, Nicolas Morey-Chaisemartin
 wrote:
> What message do you suggest ?  As I said in a previous mail, a
> simple "Editor failure, cancelling {commit, tag}" should be enough
> as launch_editor already outputs error messages describing what
> issue the editor had.
>
> I don't think suggesting moving to --no-edit || -m || -F is that
> helpful.  It's basically saying your setup is broken, but you can
> workaround by setting those options (and not saying that you're
> going to have some more issues later one).

If it's the case the launch_editor() indeed outputs an appropriate
error message, then the existing error message from tag.c is already
appropriate when --edit is not specified. It's only the --edit case
that the tag.c's additional message is somewhat weird. And, in fact,
suppressing tag.c's message might be the correct thing to do in the
--edit case:

static void create_tag(...) {
...
if (launch_editor(...)) {
   if (!opt->use_editor)
   fprintf(stderr, _("... use either -m or -F ..."));
exit(1);
}

I don't feel strongly about it either way and am fine with just
punting on the issue until someone actually complains about it.

Re: [PATCH v2 3/3] rebase: introduce and use pseudo-ref ORIG_COMMIT

[Junio C Hamano]

Duy Nguyen  writes:

> On Thu, Feb 1, 2018 at 6:18 AM, Junio C Hamano  wrote:
> ...
>> Hmph, how is this new file conceptually different from existing ones
>> like CHERRY_PICK_HEAD?
>
> Conceptually the same, except that CHERRY_PICK_HEAD can't be reused
> because it's specifically tied to git-cherry-pick (there's even code
> that delete this ref if cherry-pick is run as part of rebase, and
> git-status uses this ref to see if a cherry-pick is in progress).
> There's also REVERT_HEAD in sequencer.c, same purpose but for
> git-revert. Perhaps I should rename this new ref to REBASE_HEAD to
> follow the same naming?

I just found "ORIG_COMMIT" too similar to "ORIG_HEAD" that is
totally a different thing and feared unnecessary confusion.  I think
you are correct to suggest that REBASE_HEAD would be more in line
with the naming convention with the sequencer-like operations.

Thanks.

Your consignment box unlocking code is BGF25T660AJM,Call or text him on +1(231) 237-2231

[Rev. Dr. Paul Chimereya]

Attention Beneficiary;

Your consignment box unlocking code is BGF25T660AJM,Call or text him on +1(231) 
237-2231

 I wish to inform you that the diplomatic agent conveying the consignment box 
valued the sum of $15.8 Million United States Dollars misplaced your delivery 
address and he is currently stranded at YOUR INTERNATIONAL AIRPORT. We required 
you to reconfirm the following information as stated below for safe delivery to 
you.

Full Name: -
Nationality 
RESIDENTIAL Address -
Nearest Airport--
Direct Phone No --
Occupation--
ID ATTACHMENT-

Contact the Diplomatic agent Patrick Lennox E-mail:{lennoxpatric...@gmail.com} 
him with the information He is waiting to hear from you today with the 
information at the Airport NB:The Diplomatic agent does not know that the 
content of the consignment box is $15.8 Millions United States Dollars and on 
no circumstances should you let him know the content.Call or text him on 
+1(231) 252-1811 .You should send him email to this email address 
lennoxpatric...@gmail.com.

Thanks.
REV.DR.PAUL CHIMERE,
Managing Director DHL,
COURIER COMPANY,
TEL,+229-6952-8017.

Re: [PATCH v2 1/3] am: add --show-current-patch

[Junio C Hamano]

Duy Nguyen  writes:

> Subject: [PATCH] Preserve errno in case case before calling sth_errno()
>
> All these locations do something like this
>
> sth_errno(..., somefunc(...));
>
> where somefunc() can potentially change errno, which will be read by
> sth_errno(). Call somefunc separately with errno preserved to avoid
> this.
>
> Signed-off-by: Nguyễn Thái Ngọc Duy 
> ---
>  builtin/am.c  |  8 +++-
>  builtin/commit.c  |  8 ++--
>  builtin/init-db.c |  9 ++---
>  rerere.c  |  9 ++---
>  shallow.c | 27 ++-
>  5 files changed, 43 insertions(+), 18 deletions(-)

> diff --git a/builtin/init-db.c b/builtin/init-db.c
> index c9b7946bad..e384749f73 100644
> --- a/builtin/init-db.c
> +++ b/builtin/init-db.c
> @@ -570,9 +570,12 @@ int cmd_init_db(int argc, const char **argv, const char 
> *prefix)
>   set_git_work_tree(work_tree);
>   else
>   set_git_work_tree(git_work_tree_cfg);
> - if (access(get_git_work_tree(), X_OK))
> - die_errno (_("Cannot access work tree '%s'"),
> -get_git_work_tree());
> + if (access(get_git_work_tree(), X_OK)) {
> + int saved_errno = errno;
> + const char *path = get_git_work_tree();
> + errno = saved_errno;
> + die_errno(_("Cannot access work tree '%s'"), path);
> + }
>   }

This one is the most faithful conversion from "mechanical rewrite"
point of view, but I wonder if we should instead take the returned
path from get_git_work_tree() and use it in both calls.  After all,
this is hardly performance sensitive codepath, so even "an obviously
safe but wasteful with extra xstrdup/free" version

work_tree_path = xstrdup(get_git_work_tree());
if (access(work_tree_path, X_OK))
die_errno(_("msg..."), work_tree_path);
free(work_tree_path);

may be an improvement.

> diff --git a/rerere.c b/rerere.c
> index 1ce440f4bb..f19a53ff2c 100644
> --- a/rerere.c
> +++ b/rerere.c
> @@ -683,9 +683,12 @@ static int merge(const struct rerere_id *id, const char 
> *path)
>* A successful replay of recorded resolution.
>* Mark that "postimage" was used to help gc.
>*/
> - if (utime(rerere_path(id, "postimage"), NULL) < 0)
> - warning_errno("failed utime() on %s",
> -   rerere_path(id, "postimage"));
> + if (utime(rerere_path(id, "postimage"), NULL) < 0) {
> + int saved_errno = errno;
> + const char *path = rerere_path(id, "postimage");
> + errno = saved_errno;
> + warning_errno("failed utime() on %s", path);
> + }

Likewise.

> diff --git a/shallow.c b/shallow.c
> index df4d44ea7a..9da82f5292 100644
> --- a/shallow.c
> +++ b/shallow.c
> @@ -295,9 +295,12 @@ const char *setup_temporary_shallow(const struct 
> oid_array *extra)
>   temp = xmks_tempfile(git_path("shallow_XX"));
>  
>   if (write_in_full(temp->fd, sb.buf, sb.len) < 0 ||
> - close_tempfile_gently(temp) < 0)
> - die_errno("failed to write to %s",
> -   get_tempfile_path(temp));
> + close_tempfile_gently(temp) < 0) {
> + int saved_errno = errno;
> + const char *path = get_tempfile_path(temp);
> + errno = saved_errno;
> + die_errno("failed to write to %s", path);
> + }

It feels a bit questionable to my taste to pretend that we are truly
oblivious to how trivial get_tempfile_path() is, i.e. no more than
just a few field accesses to "tempfile" struct.  It buries more
important thing that is happening in the code in noise.

> @@ -319,9 +322,12 @@ void setup_alternate_shallow(struct lock_file 
> *shallow_lock,

Likewise.

> @@ -366,9 +372,12 @@ void prune_shallow(int show_only)

Likewise.

All others I snipped looked like good changes.  Thanks.

Re: [PATCH] cocci: simplify check for trivial format strings

[René Scharfe]

Am 02.02.2018 um 15:34 schrieb SZEDER Gábor:
> On Thu, Feb 1, 2018 at 7:56 PM, René Scharfe  wrote:
>> 353d84c537 (coccicheck: make transformation for strbuf_addf(sb, "...")
>> more precise) added a check to avoid transforming calls with format
>> strings which contain percent signs, as that would change the result.
>> It uses embedded Python code for that.  Simplify this rule by using the
>> regular expression matching operator instead.
>>
>> Signed-off-by: Rene Scharfe 
>> ---
>> Inspired by the Coccinelle package in Debian experimental, which lost
>> support for Python for some reason.  Tested only with that version
>> (1.0.6.deb-3) and Debian testing's 1.0.4.deb-3+b3.
> 
> FWIW, it appears to be working fine with Coccinelle version
> 1.0.0~rc19.deb-3 running on Travis CI.
> Applied it on top of 'rs/strbuf-cocci-workaround' currently at cd9a4b6d9
> (cocci: use format keyword instead of a literal string, 2018-01-19)
> along with an other patch to show the resulting suggestions in
> '.../*.cocci.patch' files, and the results look fine:
> 
>https://travis-ci.org/szeder/git/jobs/336573242#L1466
> 
> and are the same as without this patch:
> 
>https://travis-ci.org/szeder/git/jobs/336257153#L1466

It's good to hear that it doesn't error out, but there is no code in
master that would trigger a good or bad transformation.  It should
propose a patch for calls like this:

strbuf_addf(sb, "just a string, better use strbuf_addstr");

... but leave those examples here alone:

strbuf_addf(sb, "200%% more percent signs");
strbuf_addf(sb, "error: %m");

René

Re: Git on Windows maps creation time onto changed time

[Keith Goldfarb]

> The purpose of these values is to allow to notice a change on the file system 
> without going through the actual file data. Duplicating st_mtime would be 
> pointless.

Well, I agree with the first statement. I disagree with the 2nd. It’s not 
pointless to fix a problem, and in theory the creation date of a file can never 
change.

> Don't do that then. Use core.trustctime.

I am. Unfortunately, my problem isn’t solved by that alone. Perhaps this 
deserves its own thread, but on Windows the st_ino field is set to zero. This 
can also trigger a false positive, causing the whole cache to be rebuilt 
unnecessarily.

K.

Re: [PATCH v3 0/2] wrap format-patch diffstats around 72 columns

[Junio C Hamano]

Nguyễn Thái Ngọc Duy   writes:

> v3 fixes tests in 2/2 that I overlooked (but Jeff didn't). Interdiff:
> ...
> Nguyễn Thái Ngọc Duy (2):
>   format-patch: keep cover-letter diffstat wrapped in 72 columns
>   format-patch: reduce patch diffstat width to 72

Thanks, will replace.  I think we are pretty in good shape with
this change now.

Re: [PATCH v2 05/14] commit-graph: implement git-commit-graph --write

[Stefan Beller]

On Thu, Feb 1, 2018 at 3:33 PM, Jonathan Tan  wrote:
> On Tue, 30 Jan 2018 16:39:34 -0500
> Derrick Stolee  wrote:
>
>> diff --git a/Documentation/git-commit-graph.txt 
>> b/Documentation/git-commit-graph.txt
>> index c8ea548dfb..3f3790d9a8 100644
>> --- a/Documentation/git-commit-graph.txt
>> +++ b/Documentation/git-commit-graph.txt
>> @@ -5,3 +5,21 @@ NAME
>>  
>>  git-commit-graph - Write and verify Git commit graphs (.graph files)
>>
>> +
>> +SYNOPSIS
>> +
>> +[verse]
>> +'git commit-graph' --write  [--pack-dir ]
>
> Subcommands (like those in git submodule) generally don't take "--", as
> far as I know.

Then you know only the ugly side of Git. ;)

It is true for git-submodule and a few others (the minority of commands IIRC)
git-tag for example takes subcommands such as --list or --verify.
https://public-inbox.org/git/xmqqiomodkt9@gitster.dls.corp.google.com/

Re: how to ignore whitespace changes with --color-moved (git diff move detection)?

[Stefan Beller]

On Thu, Feb 1, 2018 at 6:13 PM, Timothee Cour  wrote:
> this PR from october 2017 was discussing a patch that'd introduce
> `--color-moved-[no-]ignore-space-change`
> https://public-inbox.org/git/20171025224620.27657-3-sbel...@google.com/
>
> however not sure what happened since then as I can't find in `git help
> diff` options even after `brew install --HEAD git`

I proposed it, but it wasn't going anywhere, because we seemed to have
a little disagreement over how a reasonable UX looks like.

The previous patch[1] in the series you link to, proposed 6 new
command line flags (3 flags + their negatives), which was deemed
inappropriate for the user ("which combination of these flags do you
need to give to get a good result?"), but I cannot find a reference for
that, I just vaguely recall that discussion.

[1] https://public-inbox.org/git/20171025224620.27657-2-sbel...@google.com/


> it's a really useful feature as it's a common use case (ppl move
> blocks and reformat in same PR)

Thanks for the encouraging words. For now you can
use '--color-moved -w' as Jeff King suggests, but that may
not be exactly what you want, because this
* creates the diff ignoring whitespaces
* and then colors the moved lines (also ignoring white spaces)
but when you want to
* obtain a real diff (no trickery with whitespaces)
* and then have some coloring aid ignoring white spaces
you are out of luck for now. Maybe you have a good idea for
the UX design? (What do we need there? Maybe an option
equivalent of `--color-moved-[no]-ignore-all-space` is sufficient
for all practical purposes?)

> If it's not merged in git repo yet is there an easy way to try out
> this feature? (even if experimental)

Can you compile Git yourself instead of installing it from homebrew?
(See "Build Git from source on OS X" on
https://www.atlassian.com/git/tutorials/install-git for example)
In that case, you can just patch your local version of Git with the
patches that you found. Download them as raw, and "git am"
them before running make.

Stefan

Re: What's cooking in git.git (Jan 2018, #04; Wed, 31)

[Junio C Hamano]

Ævar Arnfjörð Bjarmason  writes:

> On Thu, Feb 01 2018, Junio C. Hamano jotted:
>
>> * ab/wildmatch-tests (2018-01-30) 10 commits
> The 2018-01-30 series is the update mentioned in
> 87vaga9mgf@evledraar.gmail.com. You probably noticed this / just
> didn't adjust the note since you queued in in pu already, but just in
> case: the known issues in it have been resolved, but hopefully Johannes
> Schindelin can test it on Windows & report.

Thanks for a correction.  Very much appreciated.  Let's start moving
this forward then.
>> * ab/sha1dc-build (2017-12-12) 4 commits
>>  . Makefile: use the sha1collisiondetection submodule by default
>>  - sha1dc_git.h: re-arrange an ifdef chain for a subsequent change
>>  - Makefile: under "make dist", include the sha1collisiondetection submodule
>>  - Makefile: don't error out under DC_SHA1_EXTERNAL if DC_SHA1_SUBMODULE=auto
> Do you want to peel of 4/4 and just keep 1-3 should I submit another
> version without 4/4?

Nah, let's just discard the tip one without prejudice and move the
remainder forward.

Thanks.

Re: What's cooking in git.git (Jan 2018, #04; Wed, 31)

[Junio C Hamano]

Ævar Arnfjörð Bjarmason  writes:

> On Thu, Feb 01 2018, Junio C. Hamano jotted:
>
>> * nd/fix-untracked-cache-invalidation (2018-01-24) 5 commits
>>  - dir.c: stop ignoring opendir() error in open_cached_dir()
>>  - update-index doc: note a fixed bug in the untracked cache
>>  - dir.c: fix missing dir invalidation in untracked code
>>  - dir.c: avoid stat() in valid_cached_dir()
>>  - status: add a failing test showing a core.untrackedCache bug
>>
>>  Some bugs around "untracked cache" feature have been fixed.
>>
>>  Will merge to 'next'.
>
> The "update-index doc: note a fixed bug in the untracked cache" needs to
> be amended so it doesn't say "Before 2.16, ":

True; we could just say "earlier", but I am inclined to suggest that
we get drop it altogether.  Description of historical bugs is of no
interest with the version that already fixes them, so the _only_
value the doc update adds is to tell readers that the untracked
cache feature is still not well proven, and core.untrackedCache may
serve as an escape hatch from its bugs by disabling the mechanism
added for the feature.  I am *not* opposed to a replacement of the
patch that just says something like "This feature has been cause of
bugs even in recent versions of Git, and you may want to disable it
as a workaround when you are hit by an otherwise undiscovered bug in
this area", though.

Re: Git on Windows maps creation time onto changed time

[Johannes Sixt]

Am 02.02.2018 um 00:10 schrieb Keith Goldfarb:

Dear git,

While tracking down a problem with a filesystem shared by Windows and Ubuntu, I 
came across the following code in compat/mingw.c (ming_fstat(), also in 
do_lstat()):

if (GetFileInformationByHandle(fh, )) {
buf->st_ino = 0;
buf->st_gid = 0;
buf->st_uid = 0;
buf->st_nlink = 1;
buf->st_mode = file_attr_to_st_mode(fdata.dwFileAttributes);
buf->st_size = fdata.nFileSizeLow |
(((off_t)fdata.nFileSizeHigh)<<32);
buf->st_dev = buf->st_rdev = 0; /* not used by Git */
buf->st_atime = filetime_to_time_t(&(fdata.ftLastAccessTime));
buf->st_mtime = filetime_to_time_t(&(fdata.ftLastWriteTime));
buf->st_ctime = filetime_to_time_t(&(fdata.ftCreationTime));
return 0;
}

The assignment of buf->st_ctime doesn’t seem right to me. I
understand there’s no good choice here, but I think a better choice
would be to  duplicate the definition used for st_mtime.


The purpose of these values is to allow to notice a change on the file 
system without going through the actual file data. Duplicating st_mtime 
would be pointless.



Background: When I do a git status on Windows and then later on
Ubuntu (or the other order), it is extremely slow, as the entire tree
is being traversed. I tracked it down to this difference in
definition of c_time. Yes, I know about the core.trustctime variable,
but my problem aside  this seems like an unwise choice.


Don't do that then. Use core.trustctime.

-- Hannes

Re: [PATCHv2] tag: add --edit option

[Nicolas Morey-Chaisemartin]

Le 02/02/2018 à 10:57, Eric Sunshine a écrit :
> On Fri, Feb 2, 2018 at 2:15 AM, Nicolas Morey-Chaisemartin
>  wrote:
>> Le 02/02/2018 à 02:29, Eric Sunshine a écrit :
>>> On Thu, Feb 1, 2018 at 12:21 PM, Nicolas Morey-Chaisemartin
>>>  wrote:
 - I'll post another series to fix the misleading messages in both commit.c 
 and tag.c when launch_editor fails
>>> Typically, it's easier on Junio, from a patch management standpoint,
>>> if you submit all these related patches as a single series.
>>> Alternately, if you do want to submit those changes separately, before
>>> the current patch lands in "master", be sure to mention atop which
>>> patch (this one) the additional patch(es) should live. Thanks.
>> Well this patch does not touch any of the line concerned by fixing the error 
>> message. So both should be able to land in any order.
> Yup, that's a reasonable way to look at it. I see them as related
> (thus a potential patch series) simply because the existing error
> message in tag.c is fine, as is, until the --edit option is
> introduced, after which it becomes a bit iffy. Not a big deal, though.

OK ok I'll do it :)
What message do you suggest ?
As I said in a previous mail, a simple "Editor failure, cancelling {commit, 
tag}" should be enough as launch_editor already outputs error messages 
describing what issue the editor had.

I don't think suggesting moving to --no-edit || -m || -F is that helpful.
It's basically saying your setup is broken, but you can workaround by setting 
those options (and not saying that you're going to have some more issues later 
one).

>> Plus I've never had to look into localization yet so I'm going to screw up 
>> on the first few submissions (not counting on people that disagree or would 
>> prefer another message),
> As long as you just change the content of double-quoted string, you
> shouldn't have to worry about localization. The localization folks
> will handle the .po files and whatnot.

Sounds easy enough :)

Nicolas

Re: [PATCH v2 10/14] commit-graph: add core.commitgraph setting

[SZEDER Gábor]

> The commit graph feature is controlled by the new core.commitgraph config
> setting. This defaults to 0, so the feature is opt-in.
> 
> The intention of core.commitgraph is that a user can always stop checking
> for or parsing commit graph files if core.commitgraph=0.
> 
> Signed-off-by: Derrick Stolee 
> ---
>  Documentation/config.txt | 3 +++
>  cache.h  | 1 +
>  config.c | 5 +
>  environment.c| 1 +
>  4 files changed, 10 insertions(+)

Please squash this in to keep the completion script up to date.


  -- >8 --

diff --git a/contrib/completion/git-completion.bash 
b/contrib/completion/git-completion.bash
index 3683c772c..53880f627 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -2419,6 +2419,7 @@ _git_config ()
core.bigFileThreshold
core.checkStat
core.commentChar
+   core.commitGraph
core.compression
core.createObject
core.deltaBaseCacheLimit

Re: [PATCH v2 04/14] commit-graph: implement construct_commit_graph()

[SZEDER Gábor]

> Teach Git to write a commit graph file by checking all packed objects
> to see if they are commits, then store the file in the given pack
> directory.

I'm afraid that scanning all packed objects is a bit of a roundabout
way to approach this.

In my git repo, with 9 pack files at the moment, i.e. not that big a
repo and not that many pack files:

  $ time ./git commit-graph --write --update-head
  4df41a3d1cc408b7ad34bea87b51ec4ccbf4b803

  real0m27.550s
  user0m27.113s
  sys 0m0.376s

In comparison, performing a good old revision walk to gather all the
info that is written into the graph file:

  $ time git log --all --topo-order --format='%H %T %P %cd' |wc -l
  52954

  real0m0.903s
  user0m0.972s
  sys 0m0.058s



> +char* get_commit_graph_filename_hash(const char *pack_dir,
> +  struct object_id *hash)
> +{
> + size_t len;
> + struct strbuf head_path = STRBUF_INIT;
> + strbuf_addstr(_path, pack_dir);
> + strbuf_addstr(_path, "/graph-");
> + strbuf_addstr(_path, oid_to_hex(hash));
> + strbuf_addstr(_path, ".graph");

Nit: this is assembling the path of a graph file, not that of a
graph-head, so the strbuf should be renamed accordingly.

> +
> + return strbuf_detach(_path, );
> +}

Re: [PATCH v2 09/14] commit-graph: teach git-commit-graph --delete-expired

[SZEDER Gábor]

> Teach git-commit-graph to delete the graph previously referenced by 
> 'graph_head'
> when writing a new graph file and updating 'graph_head'. This prevents
> data creep by storing a list of useless graphs. Be careful to not delete
> the graph if the file did not change.

We have to be careful with deleting the previously referenced graph
file right away after generating the new one.  Consider two processes
running concurrently, one writing new graph files with
--delete-expire', and the other reading the commit graph, e.g. a
future graph-aware 'git gc' and 'git log --topo-order':

  1. 'log' reads the hash of the graph file from graph-head.
  2. 'gc' writes the new graph and graph head files and deletes the
 old graph file.
  3. 'log' tries to open the the graph file with the hash it just
 read, but that file is already gone.

At this point 'log' could simply error out, but that would be rather
unfriendly.  Or it could try harder and could just ignore the missing
graph file and walk revisions the old school way.  It would be slower,
depending on the history size maybe much slower, but it would work.
Good.

However, in addition to the reader trying harder, I think we should
also consider making the writer more careful, too, and only delete a
stale graph file after a certain grace period is elapsed; similar to
how 'git gc' only deletes old loose objects.  And then perhaps it
should delete all graph files that are older than that grace period;
as it is, neither '--clear' nor '--delete-expired' seem to care about
graph files that aren't or weren't referenced by the graph-head.


> diff --git a/builtin/commit-graph.c b/builtin/commit-graph.c
> index 4970dec133..766f09e6fc 100644
> --- a/builtin/commit-graph.c
> +++ b/builtin/commit-graph.c

> @@ -121,6 +122,17 @@ static int graph_write(void)
>   if (graph_hash)
>   printf("%s\n", oid_to_hex(graph_hash));
>  
> +
> + if (opts.delete_expired && opts.update_head && opts.has_existing &&
> + oidcmp(graph_hash, _graph_hash)) {
> + char *old_path = get_commit_graph_filename_hash(opts.pack_dir,
> + 
> _graph_hash);
> + if (remove_path(old_path))
> + die("failed to remove path %s", old_path);
> +
> + free(old_path);
> + }
> +
>   free(graph_hash);
>   return 0;
>  }
> @@ -139,6 +151,8 @@ int cmd_commit_graph(int argc, const char **argv, const 
> char *prefix)
>   N_("write commit graph file")),
>   OPT_BOOL('u', "update-head", _head,
>   N_("update graph-head to written graph file")),
> + OPT_BOOL('d', "delete-expired", _expired,
> + N_("delete expired head graph file")),
>   { OPTION_STRING, 'H', "graph-hash", _hash,
>   N_("hash"),
>   N_("A hash for a specific graph file in the pack-dir."),

Like '--update-head', '--delete-expired' is silently ignored when it's
not used with '--write'.


> diff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh
> index 6e3b62b754..b56a6d4217 100755
> --- a/t/t5318-commit-graph.sh
> +++ b/t/t5318-commit-graph.sh

> +test_expect_success 'write graph with merges' \
> +'graph3=$(git commit-graph --write --update-head --delete-expired) &&
> + test_path_is_file ${packdir}/graph-${graph3}.graph &&
> + test_path_is_missing ${packdir}/graph-${graph2}.graph &&
> + test_path_is_file ${packdir}/graph-${graph1}.graph &&
> + test_path_is_file ${packdir}/graph-head &&
> + echo ${graph3} >expect &&
> + cmp -n 40 expect ${packdir}/graph-head &&

printf and test_cmp.

> + git commit-graph --read --graph-hash=${graph3} >output &&
> + _graph_read_expect "23" "${packdir}" &&
> + cmp expect output'
> +
> +test_expect_success 'write graph with nothing new' \
> +'graph4=$(git commit-graph --write --update-head --delete-expired) &&
> + test_path_is_file ${packdir}/graph-${graph4}.graph &&
> + test_path_is_file ${packdir}/graph-${graph1}.graph &&
> + test_path_is_file ${packdir}/graph-head &&
> + echo ${graph4} >expect &&
> + cmp -n 40 expect ${packdir}/graph-head &&

Likewise.

> + git commit-graph --read --graph-hash=${graph4} >output &&
> + _graph_read_expect "23" "${packdir}" &&
> + cmp expect output'
> +
>  test_expect_success 'clear graph' \
>  'git commit-graph --clear &&
>   test_path_is_missing ${packdir}/graph-${graph2}.graph &&
> + test_path_is_file ${packdir}/graph-${graph1}.graph &&
>   test_path_is_missing ${packdir}/graph-head'
>  
>  test_expect_success 'setup bare repo' \
> @@ -121,7 +185,7 @@ test_expect_success 'write graph in bare repo' \
>   echo ${graphbare} >expect &&
>   cmp -n 40 expect ${baredir}/graph-head &&
>   git commit-graph --read --graph-hash=${graphbare} >output &&
> - _graph_read_expect "18" "${baredir}" &&
> +

Re: [PATCH] cocci: simplify check for trivial format strings

[SZEDER Gábor]

On Thu, Feb 1, 2018 at 7:56 PM, René Scharfe  wrote:
> 353d84c537 (coccicheck: make transformation for strbuf_addf(sb, "...")
> more precise) added a check to avoid transforming calls with format
> strings which contain percent signs, as that would change the result.
> It uses embedded Python code for that.  Simplify this rule by using the
> regular expression matching operator instead.
>
> Signed-off-by: Rene Scharfe 
> ---
> Inspired by the Coccinelle package in Debian experimental, which lost
> support for Python for some reason.  Tested only with that version
> (1.0.6.deb-3) and Debian testing's 1.0.4.deb-3+b3.

FWIW, it appears to be working fine with Coccinelle version
1.0.0~rc19.deb-3 running on Travis CI.
Applied it on top of 'rs/strbuf-cocci-workaround' currently at cd9a4b6d9
(cocci: use format keyword instead of a literal string, 2018-01-19)
along with an other patch to show the resulting suggestions in
'.../*.cocci.patch' files, and the results look fine:

  https://travis-ci.org/szeder/git/jobs/336573242#L1466

and are the same as without this patch:

  https://travis-ci.org/szeder/git/jobs/336257153#L1466


>
>  contrib/coccinelle/strbuf.cocci | 17 +
>  1 file changed, 1 insertion(+), 16 deletions(-)
>
> diff --git a/contrib/coccinelle/strbuf.cocci b/contrib/coccinelle/strbuf.cocci
> index 6fe8727421..e34eada1ad 100644
> --- a/contrib/coccinelle/strbuf.cocci
> +++ b/contrib/coccinelle/strbuf.cocci
> @@ -1,21 +1,6 @@
>  @ strbuf_addf_with_format_only @
>  expression E;
> -constant fmt;
> -@@
> -  strbuf_addf(E,
> -(
> -  fmt
> -|
> -  _(fmt)
> -)
> -  );
> -
> -@ script:python @
> -fmt << strbuf_addf_with_format_only.fmt;
> -@@
> -cocci.include_match("%" not in fmt)
> -
> -@ extends strbuf_addf_with_format_only @
> +constant fmt !~ "%";
>  @@
>  - strbuf_addf
>  + strbuf_addstr
> --
> 2.16.1

Re: [PATCH 7/7] worktree remove: allow it when $GIT_WORK_TREE is already gone

[Eric Sunshine]

On Wed, Jan 24, 2018 at 4:53 AM, Nguyễn Thái Ngọc Duy  wrote:
> [...]
> - $GIT_WORK_TREE _can_ be missing if the worktree is locked. In that
>   case we must not delete $GIT_DIR because the real $GIT_WORK_TREE may
>   be in a usb stick somewhere. This is already handled because we
>   check for lock first.
> [...]
>
> Noticed-by: Kaartic Sivaraam 
> Signed-off-by: Nguyễn Thái Ngọc Duy 
> ---
> diff --git a/t/t2028-worktree-move.sh b/t/t2028-worktree-move.sh
> @@ -116,4 +116,12 @@ test_expect_success 'force remove worktree with 
> untracked file' '
> +test_expect_success 'remove missing worktree' '
> +   git worktree add to-be-gone &&
> +   test -d .git/worktrees/to-be-gone &&
> +   mv to-be-gone gone &&
> +   git worktree remove to-be-gone &&
> +   test_path_is_missing .git/worktrees/to-be-gone
> +'

Perhaps there could also be a test to verify that a missing but locked
worktree is _not_ removed?

Re: [PATCH 6/7] worktree remove: new command

[Eric Sunshine]

On Wed, Jan 24, 2018 at 4:53 AM, Nguyễn Thái Ngọc Duy  wrote:
> This command allows to delete a worktree. Like 'move' you cannot
> remove the main worktree, or one with submodules inside [1].
> [...]
> Signed-off-by: Nguyễn Thái Ngọc Duy 
> ---
> diff --git a/builtin/worktree.c b/builtin/worktree.c
> @@ -688,6 +689,132 @@ static int move_worktree(int ac, const char **av, const 
> char *prefix)
> +static void check_clean_worktree(struct worktree *wt,
> +const char *original_path)
> +{
> +   [...]
> +   validate_no_submodules(wt);

It's slightly strange seeing worktree validation in a function
checking whether the worktree is clean since submodule validation
isn't an issue of cleanliness. I'd have expected the caller to invoke
it instead:

int remove_worktree(...) {
...
if (!force) {
validate_no_submodules(wt);
check_clean_worktree(wt, av[0]);
}
...
}

On the other hand, I could imagine it being called here as appropriate
if submodule validation eventually also checks submodule cleanliness
as hinted by the commit message.

> +   argv_array_pushf(_env, "%s=%s/.git",
> +GIT_DIR_ENVIRONMENT, wt->path);
> +   argv_array_pushf(_env, "%s=%s",
> +GIT_WORK_TREE_ENVIRONMENT, wt->path);
> +   memset(, 0, sizeof(cp));
> +   argv_array_pushl(, "status",
> +"--porcelain", "--ignore-submodules=none",
> +NULL);
> +   cp.env = child_env.argv;
> +   cp.git_cmd = 1;
> +   cp.dir = wt->path;
> +   cp.out = -1;
> +   ret = start_command();
> +   if (ret)
> +   die_errno(_("failed to run git-status on '%s'"),
> + original_path);

Minor: I think there was some effort recently to remove "git-foo"
style mentions from documentation and error messages. Perhaps this
could be "failed to run 'git status' on '%s'". Ditto below.

> +   ret = xread(cp.out, buf, sizeof(buf));
> +   if (ret)
> +   die(_("'%s' is dirty, use --force to delete it"),
> +   original_path);
> +   close(cp.out);
> +   ret = finish_command();
> +   if (ret)
> +   die_errno(_("failed to run git-status on '%s', code %d"),
> + original_path, ret);
> +}
> +
> +static int delete_git_work_tree(struct worktree *wt)
> +{
> +   struct strbuf sb = STRBUF_INIT;
> +   int ret = 0;
> +
> +   strbuf_addstr(, wt->path);
> +   if (remove_dir_recursively(, 0)) {
> +   error_errno(_("failed to delete '%s'"), sb.buf);
> +   ret = -1;
> +   }
> +   strbuf_release();
> +
> +   return ret;
> +}

Style nit: In the very similar delete_git_dir(), just below, there is
no blank line before 'return'.

> +
> +static int delete_git_dir(struct worktree *wt)
> +{
> +   struct strbuf sb = STRBUF_INIT;
> +   int ret = 0;
> +
> +   strbuf_addstr(, git_common_path("worktrees/%s", wt->id));
> +   if (remove_dir_recursively(, 0)) {
> +   error_errno(_("failed to delete '%s'"), sb.buf);
> +   ret = -1;
> +   }
> +   strbuf_release();
> +   return ret;
> +}
> +
> +static int remove_worktree(int ac, const char **av, const char *prefix)
> +{
> +   [...]
> +   if (reason) {
> +   if (*reason)
> +   die(_("cannot remove a locked working tree, lock 
> reason: %s"),
> +   reason);
> +   die(_("cannot remove a locked working tree"));
> +   }
> +   if (validate_worktree(wt, ))
> +   die(_("validation failed, cannot remove working tree:\n%s"),
> +   errmsg.buf);

Minor: Same concern as in 3/7 about the multi-line error message
making scripted handling of error message collection more difficult.

> +   strbuf_release();
> +
> +   if (!force)
> +   check_clean_worktree(wt, av[0]);
> +
> +   ret |= delete_git_work_tree(wt);
> +   /*
> +* continue on even if ret is non-zero, there's no going back
> +* from here.
> +*/
> +   ret |= delete_git_dir(wt);
> +
> +   free_worktrees(worktrees);
> +   return ret;
> +}
> diff --git a/t/t2028-worktree-move.sh b/t/t2028-worktree-move.sh
> @@ -90,4 +90,30 @@ test_expect_success 'move main worktree' '
> +test_expect_success 'remove locked worktree' '
> +   git worktree lock destination &&
> +   test_must_fail git worktree remove destination &&
> +   git worktree unlock destination
> +'

Perhaps place 'unlock' in test_when_finished()[1].

> +test_expect_success 'remove worktree with dirty tracked file' '
> +   echo dirty >>destination/init.t &&
> +   test_must_fail git worktree remove destination
> +'
> +
> +test_expect_success 'remove worktree with untracked file' '
> +   git -C destination 

Re: [PATCH 3/7] worktree move: new command

[Eric Sunshine]

On Fri, Feb 2, 2018 at 4:15 AM, Eric Sunshine  wrote:
> On Wed, Jan 24, 2018 at 4:53 AM, Nguyễn Thái Ngọc Duy  
> wrote:
>> +static int move_worktree(int ac, const char **av, const char *prefix)
>> +{
>> +   [...]
>> +   worktrees = get_worktrees(0);
>> +   wt = find_worktree(worktrees, prefix, av[0]);
>> +   if (!wt)
>> +   die(_("'%s' is not a working tree"), av[0]);
>
> This is still leaking 'worktrees'[1]. You probably want
> free_worktrees() immediately after the find_worktree() invocation.

Sorry, free_worktrees() after the last use of 'wt' since you still
need to access its fields, which would be the end of the function.

Re: [PATCH 5/7] worktree move: refuse to move worktrees with submodules

[Eric Sunshine]

On Wed, Jan 24, 2018 at 4:53 AM, Nguyễn Thái Ngọc Duy  wrote:
> Submodules contains .git files with relative paths. After a worktree
> move, these files need to be updated or they may point to nowhere.
>
> This is a bandage patch to make sure "worktree move" don't break
> people's worktrees by accident. When .git file update code is in
> place, this validate_no_submodules() could be removed.
>
> Signed-off-by: Nguyễn Thái Ngọc Duy 
> ---
> diff --git a/builtin/worktree.c b/builtin/worktree.c
> @@ -606,6 +606,27 @@ static int unlock_worktree(int ac, const char **av, 
> const char *prefix)
> +static void validate_no_submodules(const struct worktree *wt)
> +{
> +   struct index_state istate = { NULL };
> +   int i, found_submodules = 0;
> +
> +   if (read_index_from(, worktree_git_path(wt, "index")) > 0) {
> +   for (i = 0; i < istate.cache_nr; i++) {
> +   struct cache_entry *ce = istate.cache[i];
> +
> +   if (S_ISGITLINK(ce->ce_mode)) {
> +   found_submodules = 1;
> +   break;
> +   }
> +   }
> +   }
> +   discard_index();
> +
> +   if (found_submodules)
> +   die(_("working trees containing submodules cannot be moved"));

Minor (not worth a re-roll): This could be simplified slightly by
die()ing inside the loop rather than having 'found_submodules' and
breaking from the loop. Doing so will leak 'istate', but it's die()ing
anyhow, so should not be an issue (unless this code is someday
libified).

> +}

Re: [PATCHv2] tag: add --edit option

[Eric Sunshine]

On Fri, Feb 2, 2018 at 2:15 AM, Nicolas Morey-Chaisemartin
 wrote:
> Le 02/02/2018 à 02:29, Eric Sunshine a écrit :
>> On Thu, Feb 1, 2018 at 12:21 PM, Nicolas Morey-Chaisemartin
>>  wrote:
>>> - I'll post another series to fix the misleading messages in both commit.c 
>>> and tag.c when launch_editor fails
>> Typically, it's easier on Junio, from a patch management standpoint,
>> if you submit all these related patches as a single series.
>> Alternately, if you do want to submit those changes separately, before
>> the current patch lands in "master", be sure to mention atop which
>> patch (this one) the additional patch(es) should live. Thanks.
>
> Well this patch does not touch any of the line concerned by fixing the error 
> message. So both should be able to land in any order.

Yup, that's a reasonable way to look at it. I see them as related
(thus a potential patch series) simply because the existing error
message in tag.c is fine, as is, until the --edit option is
introduced, after which it becomes a bit iffy. Not a big deal, though.

> Plus I've never had to look into localization yet so I'm going to screw up on 
> the first few submissions (not counting on people that disagree or would 
> prefer another message),

As long as you just change the content of double-quoted string, you
shouldn't have to worry about localization. The localization folks
will handle the .po files and whatnot.

Re: [PATCH v2 1/3] am: add --show-current-patch

[Duy Nguyen]

On Fri, Feb 2, 2018 at 4:46 PM, Eric Sunshine  wrote:
> On Fri, Feb 2, 2018 at 4:25 AM, Duy Nguyen  wrote:
>> On Wed, Jan 31, 2018 at 02:59:32PM -0800, Junio C Hamano wrote:
>>> Eric Sunshine  writes:
>>> > On Wed, Jan 31, 2018 at 4:30 AM, Nguyễn Thái Ngọc Duy  
>>> > wrote:
>>> >> +   len = strbuf_read_file(, am_path(state, msgnum(state)), 0);
>>> >> +   if (len < 0)
>>> >> +   die_errno(_("failed to read '%s'"),
>>> >> + am_path(state, msgnum(state)));
>>> >
>>> > Isn't this am_path() invocation inside die_errno() likely to clobber
>>> > the 'errno' from strbuf_read_file() which you want to be reporting?
>>> True.
>>
>> Thanks both. Good catch. Of course I will fix this in the re-roll, but
>> should we also do something for the current code base with the
>> following patch?
>>
>> -   die_errno(_("could not read '%s'"), am_path(state, file));
>> +   saved_errno = errno;
>> +   path = am_path(state, file);
>> +   errno = saved_errno;
>> +   die_errno(_("could not read '%s'"), path);
>
> Rather than worrying about catching these at review time, I had been
> thinking about a solution which automates it using variadic macros.
> Something like:
>
> #define die_errno(...) do { \
> int saved_errno_ = errno; \
> die_errno_(saved_errno_, __VA_ARGS__); \
> } while (0);

That would be best. Unfortunately we have HAVE_VARIADIC_MACROS so we
need to deal with no variadic macro support too.
-- 
Duy

Re: Git remote helper's fast export options

[Mark Nauwelaerts]

On 01/02/18 00:14, Hareesh Rajan wrote:

Hi,

I use git remote helper to export/import repository content. Git
internally uses fast-export command and it seems like the options to
detect move and copy (-M/-C) are not being used.
Logging this issue requesting a fix to the remote helper to generate
rename and copy commands in the output dump.

Git version: 2.14.1

Thanks,
Hareesh
This may have to do with a few changes that happened there recently; see 
https://public-inbox.org/git/1514112729-31647-1-git-send-email-mark.nauwelae...@gmail.com/ 
for a few patches addressing that.


Mark.

Re: [PATCH v2 1/3] am: add --show-current-patch

[Eric Sunshine]

On Fri, Feb 2, 2018 at 4:25 AM, Duy Nguyen  wrote:
> On Wed, Jan 31, 2018 at 02:59:32PM -0800, Junio C Hamano wrote:
>> Eric Sunshine  writes:
>> > On Wed, Jan 31, 2018 at 4:30 AM, Nguyễn Thái Ngọc Duy  
>> > wrote:
>> >> +   len = strbuf_read_file(, am_path(state, msgnum(state)), 0);
>> >> +   if (len < 0)
>> >> +   die_errno(_("failed to read '%s'"),
>> >> + am_path(state, msgnum(state)));
>> >
>> > Isn't this am_path() invocation inside die_errno() likely to clobber
>> > the 'errno' from strbuf_read_file() which you want to be reporting?
>> True.
>
> Thanks both. Good catch. Of course I will fix this in the re-roll, but
> should we also do something for the current code base with the
> following patch?
>
> -   die_errno(_("could not read '%s'"), am_path(state, file));
> +   saved_errno = errno;
> +   path = am_path(state, file);
> +   errno = saved_errno;
> +   die_errno(_("could not read '%s'"), path);

Rather than worrying about catching these at review time, I had been
thinking about a solution which automates it using variadic macros.
Something like:

#define die_errno(...) do { \
int saved_errno_ = errno; \
die_errno_(saved_errno_, __VA_ARGS__); \
} while (0);

Re: [PATCH 2/7] worktree.c: add update_worktree_location()

[Duy Nguyen]

On Fri, Feb 2, 2018 at 3:23 PM, Eric Sunshine  wrote:
> On Wed, Jan 24, 2018 at 4:53 AM, Nguyễn Thái Ngọc Duy  
> wrote:
>> diff --git a/worktree.c b/worktree.c
>> @@ -326,6 +326,24 @@ int validate_worktree(const struct worktree *wt, struct 
>> strbuf *errmsg)
>> +void update_worktree_location(struct worktree *wt, const char *path_)
>> +{
>> +   struct strbuf path = STRBUF_INIT;
>> +
>> +   if (is_main_worktree(wt))
>> +   die("BUG: can't relocate main worktree");
>> +
>> +   strbuf_add_absolute_path(, path_);
>> +   if (fspathcmp(wt->path, path.buf)) {
>> +   write_file(git_common_path("worktrees/%s/gitdir",
>> +  wt->id),
>> +  "%s/.git", real_path(path.buf));
>
> For the path stored in 'worktrees//gitdir' (and in wt->path), this
> and other worktree-related code sometimes treats it only as "absolute
> path" and sometimes as "real path". As a reviewer, I'm having trouble
> understanding the logic of why, how, and when this distinction is
> made. Can you explain a bit to help clarify when "absolute path" is
> good enough and when "real path" is needed?

Of the top of my head, I think it's "anything but a relative path".
real_path() is normally used to normalize paths before I compare them.
Writing a normalized path down is nice to avoid "../" but I don't
think it's strictly necessary. Well, it's also prettier when you have
to print the path out. Printing "/foo/bar/abc/../../xyz" is not nice.

Hmm.. looking back in add_worktree(), we also write "gitdir" with
real_path(). I think the problem here is that I should have
real_path()'d "path.buf" _before_ doing fspathcmp(). Maybe that's
where the confusion's from.
-- 
Duy

Re: [PATCH 4/7] worktree move: accept destination as directory

[Eric Sunshine]

On Wed, Jan 24, 2018 at 4:53 AM, Nguyễn Thái Ngọc Duy  wrote:
> Similar to "mv a b/", which is actually "mv a b/a", we extract basename
> of source worktree and create a directory of the same name at
> destination if dst path is a directory.
>
> Signed-off-by: Nguyễn Thái Ngọc Duy 
> ---
> diff --git a/builtin/worktree.c b/builtin/worktree.c
> @@ -624,8 +624,6 @@ static int move_worktree(int ac, const char **av, const 
> char *prefix)
> path = prefix_filename(prefix, av[1]);
> strbuf_addstr(, path);
> free(path);
> -   if (file_exists(dst.buf))
> -   die(_("target '%s' already exists"), av[1]);

Nit: The distance between this "removed" conditional and the code
inserted below hampered review a bit since it made it slightly more
onerous to check for unwanted logic changes. Had patch 3/7 located
this conditional below the is_main_worktree() check (where the new
code is inserted below), this 'if' would have merely mutated into an
'else if' but not otherwise moved, thus review would have been
simplified. (Not itself worth a re-roll.)

> worktrees = get_worktrees(0);
> wt = find_worktree(worktrees, prefix, av[0]);
> @@ -633,6 +631,20 @@ static int move_worktree(int ac, const char **av, const 
> char *prefix)
> die(_("'%s' is not a working tree"), av[0]);
> if (is_main_worktree(wt))
> die(_("'%s' is a main working tree"), av[0]);
> +
> +   if (is_directory(dst.buf)) {
> +   const char *sep = find_last_dir_sep(wt->path);
> +
> +   if (!sep)
> +   die(_("could not figure out destination name from 
> '%s'"),
> +   wt->path);
> +   strbuf_addstr(, sep);

Do we know at this point whether 'dst' has a terminating "/"? If it
does, then this addstr() could result in "//" in the path which might
be problematic on Windows.

> +   if (file_exists(dst.buf))
> +   die(_("target '%s' already exists"), dst.buf);
> +   } else if (file_exists(dst.buf)) {
> +   die(_("target '%s' already exists"), av[1]);
> +   }

I wonder if it makes sense to collapse the duplicated
file_exists()/"target already exists" code?

if (is_directory(...)) {
...
strbuf_addstr(, sep);
}
if (file_exists(dst.buf))
die(_("target '%s' already exists"), dst.buf);

It changes the error message slightly for the non-directory case but
simplifies the code a bit.

> reason = is_worktree_locked(wt);
> if (reason) {
> if (*reason)

Perhaps add a test to t2028-worktree-move.sh to show that this new
behavior works?

Re: [PATCHv2] tag: add --edit option

[Nicolas Morey-Chaisemartin]

Le 02/02/2018 à 02:29, Eric Sunshine a écrit :
> On Thu, Feb 1, 2018 at 12:21 PM, Nicolas Morey-Chaisemartin
>  wrote:
>> Add a --edit option whichs allows modifying the messages provided by -m or 
>> -F,
>> the same way git commit --edit does.
>>
>> Signed-off-by: Nicolas Morey-Chaisemartin 
>> ---
>> Changes since v1:
>> - Fix usage string
>> - Use write_script to generate editor
>> - Rename editor to fakeeditor to match the other tests in the testsuite
> Thanks for explaining what changed since the previous attempt. It is
> also helpful for reviewers if you include a reference to the previous
> iteration, like this:
> https://public-inbox.org/git/450140f4-d410-4f1a-e5c1-c56d345a7...@suse.com/T/#u
>
> Cc:'ing reviewers of previous iterations is also good etiquette when
> submitting a new version.

I thought I did. My script might be glitchy. Sorry for that.

>
>> - I'll post another series to fix the misleading messages in both commit.c 
>> and tag.c when launch_editor fails
> Typically, it's easier on Junio, from a patch management standpoint,
> if you submit all these related patches as a single series.
> Alternately, if you do want to submit those changes separately, before
> the current patch lands in "master", be sure to mention atop which
> patch (this one) the additional patch(es) should live. Thanks.

Well this patch does not touch any of the line concerned by fixing the error 
message. So both should be able to land in any order.
Plus I've never had to look into localization yet so I'm going to screw up on 
the first few submissions (not counting on people that disagree or would prefer 
another message),
and I don't want this patch to get stuck in the pipe for that :)

>
>> diff --git a/Documentation/git-tag.txt b/Documentation/git-tag.txt
>> @@ -167,6 +167,12 @@ This option is only applicable when listing tags 
>> without annotation lines.
>> +-e::
>> +--edit::
>> +   The message taken from file with `-F` and command line with
>> +   `-m` are usually used as the tag message unmodified.
>> +   This option lets you further edit the message taken from these 
>> sources.
> You probably ought to add this new option to the command synopsis. In
> the git-commit man page, the synopsis mentions only '-e' (not --edit),
> so perhaps this man page could mirror that one. (Sorry for not
> noticing this earlier.)

Yep makes sense.

>> diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh
>> @@ -452,6 +452,21 @@ test_expect_success \
>> +get_tag_header annotated-tag-edit $commit commit $time >expect
>> +echo "An edited message" >>expect
> Modern practice is to perform these "expect" setup actions (and all
> other actions) within tests themselves rather than outside of tests.
> However, consistency also has value, and since this test script is
> filled with this sort of stylized "expect" setup already, this may be
> fine, and probably not worth a re-roll. (A "modernization" patch by
> someone can come later if warranted.)
>
>> +test_expect_success 'set up editor' '
>> +   write_script fakeeditor <<-\EOF
>> +   sed -e "s/A message/An edited message/g" <"$1" >"$1-"
>> +   mv "$1-" "$1"
>> +   EOF
>> +'

It's probably worth doing a whole cleanup of these (and switch to write script) 
in a dedicated patch series.

Nicolas

Re: [PATCH v2 1/3] am: add --show-current-patch

[Duy Nguyen]

On Wed, Jan 31, 2018 at 02:59:32PM -0800, Junio C Hamano wrote:
> Eric Sunshine  writes:
> 
> > On Wed, Jan 31, 2018 at 4:30 AM, Nguyễn Thái Ngọc Duy  
> > wrote:
> >> Pointing the user to $GIT_DIR/rebase-apply may encourage them to mess
> >> around in there, which is not a good thing. With this, the user does
> >> not have to keep the path around somewhere (because after a couple of
> >> commands, the path may be out of scrollback buffer) when they need to
> >> look at the patch.
> >>
> >> Signed-off-by: Nguyễn Thái Ngọc Duy 
> >> ---
> >> diff --git a/builtin/am.c b/builtin/am.c
> >> @@ -2121,6 +2120,22 @@ static void am_abort(struct am_state *state)
> >> +static int show_patch(struct am_state *state)
> >> +{
> >> +   struct strbuf sb = STRBUF_INIT;
> >> +   int len;
> >> +
> >> +   len = strbuf_read_file(, am_path(state, msgnum(state)), 0);
> >> +   if (len < 0)
> >> +   die_errno(_("failed to read '%s'"),
> >> + am_path(state, msgnum(state)));
> >
> > Isn't this am_path() invocation inside die_errno() likely to clobber
> > the 'errno' from strbuf_read_file() which you want to be reporting?
> 
> True.

Thanks both. Good catch. Of course I will fix this in the re-roll, but
should we also do something for the current code base with the
following patch?

I think this is something coccinelle can help catch, but my
coccinelle-foo is way too low to come up with something like
"die_errno() must not take any arguments as function calls, except _()
and N_()".

-- 8< --
Subject: [PATCH] Preserve errno in case case before calling sth_errno()

All these locations do something like this

sth_errno(..., somefunc(...));

where somefunc() can potentially change errno, which will be read by
sth_errno(). Call somefunc separately with errno preserved to avoid
this.

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 builtin/am.c  |  8 +++-
 builtin/commit.c  |  8 ++--
 builtin/init-db.c |  9 ++---
 rerere.c  |  9 ++---
 shallow.c | 27 ++-
 5 files changed, 43 insertions(+), 18 deletions(-)

diff --git a/builtin/am.c b/builtin/am.c
index acfe9d3c8c..ba67e187a4 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -244,6 +244,9 @@ static int am_in_progress(const struct am_state *state)
 static int read_state_file(struct strbuf *sb, const struct am_state *state,
const char *file, int trim)
 {
+   int saved_errno;
+   const char *path;
+
strbuf_reset(sb);
 
if (strbuf_read_file(sb, am_path(state, file), 0) >= 0) {
@@ -256,7 +259,10 @@ static int read_state_file(struct strbuf *sb, const struct 
am_state *state,
if (errno == ENOENT)
return -1;
 
-   die_errno(_("could not read '%s'"), am_path(state, file));
+   saved_errno = errno;
+   path = am_path(state, file);
+   errno = saved_errno;
+   die_errno(_("could not read '%s'"), path);
 }
 
 /**
diff --git a/builtin/commit.c b/builtin/commit.c
index 4610e3d8e3..39836b3734 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -780,8 +780,12 @@ static int prepare_to_commit(const char *index_file, const 
char *prefix,
}
 
s->fp = fopen_for_writing(git_path_commit_editmsg());
-   if (s->fp == NULL)
-   die_errno(_("could not open '%s'"), git_path_commit_editmsg());
+   if (s->fp == NULL) {
+   int saved_errno = errno;
+   const char *path = git_path_commit_editmsg();
+   errno = saved_errno;
+   die_errno(_("could not open '%s'"), path);
+   }
 
/* Ignore status.displayCommentPrefix: we do need comments in 
COMMIT_EDITMSG. */
old_display_comment_prefix = s->display_comment_prefix;
diff --git a/builtin/init-db.c b/builtin/init-db.c
index c9b7946bad..e384749f73 100644
--- a/builtin/init-db.c
+++ b/builtin/init-db.c
@@ -570,9 +570,12 @@ int cmd_init_db(int argc, const char **argv, const char 
*prefix)
set_git_work_tree(work_tree);
else
set_git_work_tree(git_work_tree_cfg);
-   if (access(get_git_work_tree(), X_OK))
-   die_errno (_("Cannot access work tree '%s'"),
-  get_git_work_tree());
+   if (access(get_git_work_tree(), X_OK)) {
+   int saved_errno = errno;
+   const char *path = get_git_work_tree();
+   errno = saved_errno;
+   die_errno(_("Cannot access work tree '%s'"), path);
+   }
}
else {
if (work_tree)
diff --git a/rerere.c b/rerere.c
index 1ce440f4bb..f19a53ff2c 100644
--- a/rerere.c
+++ b/rerere.c
@@ -683,9 +683,12 @@ static int merge(const struct rerere_id *id, const char 
*path)
 * A successful replay of recorded resolution.
 

Re: [PATCH 3/7] worktree move: new command

[Eric Sunshine]

On Wed, Jan 24, 2018 at 4:53 AM, Nguyễn Thái Ngọc Duy  wrote:
> This command allows to relocate linked worktrees. Main worktree cannot
> (yet) be moved.
> [...]
> Signed-off-by: Nguyễn Thái Ngọc Duy 
> ---
> diff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt
> @@ -79,6 +80,11 @@ files from being pruned automatically. This also prevents 
> it from
> +move::
> +
> +Move a working tree to a new location. Note that the main working tree
> +cannot be moved.
> +
> @@ -281,7 +287,6 @@ performed manually, such as:
>  - `remove` to remove a linked working tree and its administrative files (and
>warn if the working tree is dirty)
> -- `mv` to move or rename a working tree and update its administrative files

A couple other places in this document ought to be updated.
Specifically, in DESCRIPTION:

If you move a linked working tree, you need to manually update the
administrative files so that they do not get pruned automatically.
See section "DETAILS" for more information.

which can probably be dropped in its entirety. And, in DETAILS:

If you move a linked working tree, you need to update the 'gitdir'
file...

should probably be reworded to

If you _manually_ move a linked working tree, you need to update
the 'gitdir' file...

> diff --git a/builtin/worktree.c b/builtin/worktree.c
> @@ -605,6 +606,53 @@ static int unlock_worktree(int ac, const char **av, 
> const char *prefix)
> +static int move_worktree(int ac, const char **av, const char *prefix)
> +{
> +   [...]
> +   worktrees = get_worktrees(0);
> +   wt = find_worktree(worktrees, prefix, av[0]);
> +   if (!wt)
> +   die(_("'%s' is not a working tree"), av[0]);

This is still leaking 'worktrees'[1]. You probably want
free_worktrees() immediately after the find_worktree() invocation.

> +   if (is_main_worktree(wt))
> +   die(_("'%s' is a main working tree"), av[0]);
> +   reason = is_worktree_locked(wt);
> +   if (reason) {
> +   if (*reason)
> +   die(_("cannot move a locked working tree, lock 
> reason: %s"),
> +   reason);
> +   die(_("cannot move a locked working tree"));
> +   }
> +   if (validate_worktree(wt, ))
> +   die(_("validation failed, cannot move working tree:\n%s"),
> +   errmsg.buf);

Minor: All the other error messages are presented on a single line.
Despite this error message potentially being quite long, it worries me
a bit that presenting it on two lines could complicate scripted
clients if they need to collect the error message for special
handling.

> diff --git a/t/t2028-worktree-move.sh b/t/t2028-worktree-move.sh
> @@ -59,4 +59,35 @@ test_expect_success 'unlock worktree twice' '
> +test_expect_success 'move locked worktree' '
> +   git worktree lock source &&
> +   test_must_fail git worktree move source destination &&
> +   git worktree unlock source
> +'

Also from [1], wrapping 'unlock' inside a test_when_finished()
invocation seems potentially desirable.

> +test_expect_success 'move worktree' '
> +   toplevel="$(pwd)" &&
> +   git worktree move source destination &&
> +   test_path_is_missing source &&
> +   git worktree list --porcelain | grep "^worktree" >actual &&
> +   cat <<-EOF >expected &&
> +   worktree $toplevel
> +   worktree $toplevel/destination
> +   worktree $toplevel/elsewhere
> +   EOF
> +   test_cmp expected actual &&

This seems somewhat fragile. If someone inserts a test before this one
which adds another worktree, then this test would break. Perhaps the
filtering of the --porcelain output could be more strict? (Not
necessarily worth a re-roll, though.)

> +   git -C destination log --format=%s >actual2 &&
> +   echo init >expected2 &&
> +   test_cmp expected2 actual2
> +'

[1]: 
https://public-inbox.org/git/CAPig+cRqgzB4tiTb=fhufbtqo3qegm3m378pvose06kdrek...@mail.gmail.com/

Re: [PATCH 2/7] worktree.c: add update_worktree_location()

[Eric Sunshine]

On Wed, Jan 24, 2018 at 4:53 AM, Nguyễn Thái Ngọc Duy  wrote:
> diff --git a/worktree.c b/worktree.c
> @@ -326,6 +326,24 @@ int validate_worktree(const struct worktree *wt, struct 
> strbuf *errmsg)
> +void update_worktree_location(struct worktree *wt, const char *path_)
> +{
> +   struct strbuf path = STRBUF_INIT;
> +
> +   if (is_main_worktree(wt))
> +   die("BUG: can't relocate main worktree");
> +
> +   strbuf_add_absolute_path(, path_);
> +   if (fspathcmp(wt->path, path.buf)) {
> +   write_file(git_common_path("worktrees/%s/gitdir",
> +  wt->id),
> +  "%s/.git", real_path(path.buf));

For the path stored in 'worktrees//gitdir' (and in wt->path), this
and other worktree-related code sometimes treats it only as "absolute
path" and sometimes as "real path". As a reviewer, I'm having trouble
understanding the logic of why, how, and when this distinction is
made. Can you explain a bit to help clarify when "absolute path" is
good enough and when "real path" is needed?

> +   free(wt->path);
> +   wt->path = strbuf_detach(, NULL);
> +   }
> +   strbuf_release();
> +}