[PATCH 5/6] fsmonitor: Remove debugging lines from t/t7519-status-fsmonitor.sh

[Alex Vandiver]

These were mistakenly left in when the test was introduced, in
1487372d3 ("fsmonitor: store fsmonitor bitmap before splitting index",
2017-11-09)

Signed-off-by: Alex Vandiver 
---
 t/t7519-status-fsmonitor.sh | 2 --
 1 file changed, 2 deletions(-)

diff --git a/t/t7519-status-fsmonitor.sh b/t/t7519-status-fsmonitor.sh
index eb2d13bbc..19b2a0a0f 100755
--- a/t/t7519-status-fsmonitor.sh
+++ b/t/t7519-status-fsmonitor.sh
@@ -307,9 +307,7 @@ test_expect_success 'splitting the index results in the 
same state' '
dirty_repo &&
git update-index --fsmonitor  &&
git ls-files -f >expect &&
-   test-dump-fsmonitor >&2 && echo &&
git update-index --fsmonitor --split-index &&
-   test-dump-fsmonitor >&2 && echo &&
git ls-files -f >actual &&
test_cmp expect actual
 '
-- 
2.15.1.626.gc4617b774

[PATCH 6/6] fsmonitor: Use fsmonitor data in `git diff`

[Alex Vandiver]

With fsmonitor enabled, the first call to match_stat_with_submodule
calls refresh_fsmonitor, incurring the overhead of reading the list of
updated files -- but run_diff_files does not respect the
CE_FSMONITOR_VALID flag.

Make use of the fsmonitor extension to skip lstat() calls on files
that fsmonitor judged as unmodified.  Skip use of the fsmonitor
extension when called by "add", as the format_callback in such cases
expects to be called even when the file is believed to be "up to date"
with the index.

Notably, this change improves performance of the git shell prompt when
GIT_PS1_SHOWDIRTYSTATE is set.

Signed-off-by: Alex Vandiver 
---
 builtin/add.c | 2 +-
 diff-lib.c| 6 ++
 diff.h| 2 ++
 3 files changed, 9 insertions(+), 1 deletion(-)

diff --git a/builtin/add.c b/builtin/add.c
index bf01d89e2..bba20b46e 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -119,7 +119,7 @@ int add_files_to_cache(const char *prefix,
rev.diffopt.format_callback_data = 
rev.diffopt.flags.override_submodule_config = 1;
rev.max_count = 0; /* do not compare unmerged paths with stage #2 */
-   run_diff_files(, DIFF_RACY_IS_MODIFIED);
+   run_diff_files(, DIFF_RACY_IS_MODIFIED | DIFF_SKIP_FSMONITOR);
clear_pathspec(_data);
return !!data.add_errors;
 }
diff --git a/diff-lib.c b/diff-lib.c
index 8104603a3..13ff00d81 100644
--- a/diff-lib.c
+++ b/diff-lib.c
@@ -95,6 +95,9 @@ int run_diff_files(struct rev_info *revs, unsigned int option)
 
diff_set_mnemonic_prefix(>diffopt, "i/", "w/");
 
+   if (!(option & DIFF_SKIP_FSMONITOR))
+   refresh_fsmonitor(_index);
+
if (diff_unmerged_stage < 0)
diff_unmerged_stage = 2;
entries = active_nr;
@@ -197,6 +200,9 @@ int run_diff_files(struct rev_info *revs, unsigned int 
option)
if (ce_uptodate(ce) || ce_skip_worktree(ce))
continue;
 
+   if (ce->ce_flags & CE_FSMONITOR_VALID && !(option & 
DIFF_SKIP_FSMONITOR))
+   continue;
+
/* If CE_VALID is set, don't look at workdir for file removal */
if (ce->ce_flags & CE_VALID) {
changed = 0;
diff --git a/diff.h b/diff.h
index 36a09624f..1060bc495 100644
--- a/diff.h
+++ b/diff.h
@@ -395,6 +395,8 @@ extern const char *diff_aligned_abbrev(const struct 
object_id *sha1, int);
 #define DIFF_SILENT_ON_REMOVED 01
 /* report racily-clean paths as modified */
 #define DIFF_RACY_IS_MODIFIED 02
+/* skip loading the fsmonitor data */
+#define DIFF_SKIP_FSMONITOR 04
 extern int run_diff_files(struct rev_info *revs, unsigned int option);
 extern int run_diff_index(struct rev_info *revs, int cached);
 
-- 
2.15.1.626.gc4617b774

Re: [FYI PATCH] t/helper/test-lazy-name-hash: fix compilation

[Jonathan Tan]

On Mon, 18 Dec 2017 13:49:47 -0800
Stefan Beller  wrote:

> I was compiling origin/master today with stricter compiler flags today
> and was greeted by
> 
> t/helper/test-lazy-init-name-hash.c: In function ‘cmd_main’:
> t/helper/test-lazy-init-name-hash.c:172:5: error: ‘nr_threads_used’ may be 
> used uninitialized in this function [-Werror=maybe-uninitialized]
>  printf("avg [size %8d] [single %f] %c [multi %f %d]\n",
>  ^~~
>  nr,
>  ~~~
>  (double)avg_single/10,
>  ~~
>  (avg_single < avg_multi ? '<' : '>'),
>  ~
>  (double)avg_multi/10,
>  ~
>  nr_threads_used);
>  
> t/helper/test-lazy-init-name-hash.c:115:6: note: ‘nr_threads_used’ was 
> declared here
>   int nr_threads_used;
>   ^~~
> 
> I do not see how we can arrive at that line without having `nr_threads_used`
> initialized, as we'd have `count > 1`  (which asserts that we ran the
> loop above at least once, such that it *should* be initialized).

Your analysis makes sense. (The compiler probably couldn't detect it because
"count" is a static variable, not a local variable.)

> --- a/t/helper/test-lazy-init-name-hash.c
> +++ b/t/helper/test-lazy-init-name-hash.c
> @@ -112,7 +112,7 @@ static void analyze_run(void)
>  {
>   uint64_t t1s, t1m, t2s, t2m;
>   int cache_nr_limit;
> - int nr_threads_used;
> + int nr_threads_used = 0;
>   int i;
>   int nr;

I agree that this is probably the best way to fix it. Another way might
be to omit printing out the number of threads used in the printf that
prints the average statistics.

The best way is probably to not use so many global variables, but that
is out of scope of this change.

Did you get my message this time?

[Friedrich Mayrhofer]

-- 

This is the second time.

I, Friedrich Mayrhofer Donate $ 1,000,000.00 to You, Email Me personally for 
more details.

Regards. 
Friedrich Mayrhofer

Bug with "git submodule update" + subrepo with differing path/name?

[Andreas Urke]

Hi,

I have a repo with two submodules. I regularly use "git submodule
update", which works fine for one subrepo, but not for the other. The
only relevant difference I can think of between these two subrepos is
that the latter one does not have a matching path and name:

$ cat .gitmodules
[submodule "subrepo1"]
path = subrepo1
url = g...@gitlab.com:subrepo1.git
[submodule "name_subrepo2"]
path = subrepo2
url = g...@gitlab.com:name_subrepo2.git

And a reproduction looks like this:

$ git --version
git version 2.15.1

$ git status
HEAD detached at 05412d4
Changes not staged for commit:

modified:   subrepo1 (new commits)
modified:   subrepo2 (new commits)

$ git submodule update
Submodule path 'subrepo1': checked out
'e4785498130ec4188bb1745f27e311bff08ed4c8'

$ git status
HEAD detached at 05412d4
Changes not staged for commit:

modified:   subrepo2  (new commits)

Checking out the relevant commit manually in subrepo2 works fine (and
is what I end up doing).

Regards,
Andreas Urke

Re: [PATCH v2 1/3] repository: fix repo_read_index with submodules

[Brandon Williams]

On 12/18, Thomas Gummerer wrote:
> On 12/18, Brandon Williams wrote:
> > On 12/17, Thomas Gummerer wrote:
> > > repo_read_index calls read_index_from, which takes an path argument for
> > > the location of the index file.  For the split index however it relies
> > 
> > > on the current working directory to construct the path using git_path.
> > 
> > This line isn't actually true and should probably be fixed.  git_path
> > doesn't rely on the CWD but rather it relies on the gitdir of the main
> > repository (the_repository).
> 
> Right, let me fix that.  Thanks!
> 
> > > 
> > > repo_read_index calls read_index_from with the full path for the index
> > > file, however it doesn't change the cwd, so when split index mode is
> > > turned on, read_index_from can't find the file for the split index.
> > > 
> > > For example t3007-ls-files-recurse-submodules.sh was broken with
> > > GIT_TEST_SPLIT_INDEX set in 188dce131f ("ls-files: use repository
> > > object", 2017-06-22), and t7814-grep-recurse-submodules.sh was also
> > > broken in a similar manner, probably by introducing struct repository
> > > there, although I didn't track down the exact commit for that.
> > > 
> > > Fix this by introducing a new read_index_for_repo function, which knows
> > > about the correct paths for the submodules.
> > > 
> > > The alternative would have been to make the callers pass in the base
> > > path for the split index, however that ended up being more complicated,
> > > and I think we want to converge towards using struct repository for
> > > things like these anyway.
> > > 
> > > Signed-off-by: Thomas Gummerer 
> > > ---
> > >  cache.h  |  1 +
> > >  read-cache.c | 16 ++--
> > >  repository.c |  2 +-
> > >  3 files changed, 16 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/cache.h b/cache.h
> > > index cb5db7bf83..d42bea1ef7 100644
> > > --- a/cache.h
> > > +++ b/cache.h
> > > @@ -614,6 +614,7 @@ extern int read_index_preload(struct index_state *, 
> > > const struct pathspec *paths
> > >  extern int do_read_index(struct index_state *istate, const char *path,
> > >int must_exist); /* for testting only! */
> > >  extern int read_index_from(struct index_state *, const char *path);
> > > +extern int read_index_for_repo(const struct repository *);
> > >  extern int is_index_unborn(struct index_state *);
> > >  extern int read_index_unmerged(struct index_state *);
> > >  
> > > diff --git a/read-cache.c b/read-cache.c
> > > index 2eb81a66b9..70357febdc 100644
> > > --- a/read-cache.c
> > > +++ b/read-cache.c
> > > @@ -20,6 +20,7 @@
> > >  #include "split-index.h"
> > >  #include "utf8.h"
> > >  #include "fsmonitor.h"
> > > +#include "repository.h"
> > >  
> > >  /* Mask for the name length in ce_flags in the on-disk index */
> > >  
> > > @@ -1871,7 +1872,8 @@ static void freshen_shared_index(char 
> > > *base_sha1_hex, int warn)
> > >   free(shared_index);
> > >  }
> > >  
> > > -int read_index_from(struct index_state *istate, const char *path)
> > > +static int do_read_index_from(struct index_state *istate, const char 
> > > *path,
> > > +   const struct repository *repo)
> > >  {
> > >   struct split_index *split_index;
> > >   int ret;
> > > @@ -1896,7 +1898,7 @@ int read_index_from(struct index_state *istate, 
> > > const char *path)
> > >   split_index->base = xcalloc(1, sizeof(*split_index->base));
> > >  
> > >   base_sha1_hex = sha1_to_hex(split_index->base_sha1);
> > > - base_path = git_path("sharedindex.%s", base_sha1_hex);
> > > + base_path = repo_git_path(repo, "sharedindex.%s", base_sha1_hex);
> > >   ret = do_read_index(split_index->base, base_path, 1);
> > >   if (hashcmp(split_index->base_sha1, split_index->base->sha1))
> > >   die("broken index, expect %s in %s, got %s",
> > > @@ -1909,6 +1911,16 @@ int read_index_from(struct index_state *istate, 
> > > const char *path)
> > >   return ret;
> > >  }
> > >  
> > > +int read_index_for_repo(const struct repository *repo)
> > > +{
> > > + return do_read_index_from(repo->index, repo->index_file, repo);
> > > +}
> > 
> > > +
> > > +int read_index_from(struct index_state *istate, const char *path)
> > > +{
> > > + return do_read_index_from(istate, path, the_repository);
> > > +}
> > 
> > This looks fine, though I wonder what the point of passing in the index
> > file even was since we end just ended up reading the 'sharedindex' file 
> > based
> > on the git path. I'm just curious about how this function evolved.
> 
> There are some callsites that are using an index different form
> $gitdir/index, or even GIT_INDEX_FILE.  e.g. see builtin/am.c [*1*],
> which uses it's own 'patch-merge-index' in the am state directory for
> its internal operations.
> 
> The split index mode was later bolted on, and the sharedindex.
> would always go in $gitdir for the repository.  Others probably know
> quite a bit more about this, while I'm always interested in index
> related 

Re: [PATCH v2 1/3] repository: fix repo_read_index with submodules

[Thomas Gummerer]

On 12/18, Brandon Williams wrote:
> On 12/17, Thomas Gummerer wrote:
> > repo_read_index calls read_index_from, which takes an path argument for
> > the location of the index file.  For the split index however it relies
> 
> > on the current working directory to construct the path using git_path.
> 
> This line isn't actually true and should probably be fixed.  git_path
> doesn't rely on the CWD but rather it relies on the gitdir of the main
> repository (the_repository).

Right, let me fix that.  Thanks!

> > 
> > repo_read_index calls read_index_from with the full path for the index
> > file, however it doesn't change the cwd, so when split index mode is
> > turned on, read_index_from can't find the file for the split index.
> > 
> > For example t3007-ls-files-recurse-submodules.sh was broken with
> > GIT_TEST_SPLIT_INDEX set in 188dce131f ("ls-files: use repository
> > object", 2017-06-22), and t7814-grep-recurse-submodules.sh was also
> > broken in a similar manner, probably by introducing struct repository
> > there, although I didn't track down the exact commit for that.
> > 
> > Fix this by introducing a new read_index_for_repo function, which knows
> > about the correct paths for the submodules.
> > 
> > The alternative would have been to make the callers pass in the base
> > path for the split index, however that ended up being more complicated,
> > and I think we want to converge towards using struct repository for
> > things like these anyway.
> > 
> > Signed-off-by: Thomas Gummerer 
> > ---
> >  cache.h  |  1 +
> >  read-cache.c | 16 ++--
> >  repository.c |  2 +-
> >  3 files changed, 16 insertions(+), 3 deletions(-)
> > 
> > diff --git a/cache.h b/cache.h
> > index cb5db7bf83..d42bea1ef7 100644
> > --- a/cache.h
> > +++ b/cache.h
> > @@ -614,6 +614,7 @@ extern int read_index_preload(struct index_state *, 
> > const struct pathspec *paths
> >  extern int do_read_index(struct index_state *istate, const char *path,
> >  int must_exist); /* for testting only! */
> >  extern int read_index_from(struct index_state *, const char *path);
> > +extern int read_index_for_repo(const struct repository *);
> >  extern int is_index_unborn(struct index_state *);
> >  extern int read_index_unmerged(struct index_state *);
> >  
> > diff --git a/read-cache.c b/read-cache.c
> > index 2eb81a66b9..70357febdc 100644
> > --- a/read-cache.c
> > +++ b/read-cache.c
> > @@ -20,6 +20,7 @@
> >  #include "split-index.h"
> >  #include "utf8.h"
> >  #include "fsmonitor.h"
> > +#include "repository.h"
> >  
> >  /* Mask for the name length in ce_flags in the on-disk index */
> >  
> > @@ -1871,7 +1872,8 @@ static void freshen_shared_index(char *base_sha1_hex, 
> > int warn)
> > free(shared_index);
> >  }
> >  
> > -int read_index_from(struct index_state *istate, const char *path)
> > +static int do_read_index_from(struct index_state *istate, const char *path,
> > + const struct repository *repo)
> >  {
> > struct split_index *split_index;
> > int ret;
> > @@ -1896,7 +1898,7 @@ int read_index_from(struct index_state *istate, const 
> > char *path)
> > split_index->base = xcalloc(1, sizeof(*split_index->base));
> >  
> > base_sha1_hex = sha1_to_hex(split_index->base_sha1);
> > -   base_path = git_path("sharedindex.%s", base_sha1_hex);
> > +   base_path = repo_git_path(repo, "sharedindex.%s", base_sha1_hex);
> > ret = do_read_index(split_index->base, base_path, 1);
> > if (hashcmp(split_index->base_sha1, split_index->base->sha1))
> > die("broken index, expect %s in %s, got %s",
> > @@ -1909,6 +1911,16 @@ int read_index_from(struct index_state *istate, 
> > const char *path)
> > return ret;
> >  }
> >  
> > +int read_index_for_repo(const struct repository *repo)
> > +{
> > +   return do_read_index_from(repo->index, repo->index_file, repo);
> > +}
> 
> > +
> > +int read_index_from(struct index_state *istate, const char *path)
> > +{
> > +   return do_read_index_from(istate, path, the_repository);
> > +}
> 
> This looks fine, though I wonder what the point of passing in the index
> file even was since we end just ended up reading the 'sharedindex' file based
> on the git path. I'm just curious about how this function evolved.

There are some callsites that are using an index different form
$gitdir/index, or even GIT_INDEX_FILE.  e.g. see builtin/am.c [*1*],
which uses it's own 'patch-merge-index' in the am state directory for
its internal operations.

The split index mode was later bolted on, and the sharedindex.
would always go in $gitdir for the repository.  Others probably know
quite a bit more about this, while I'm always interested in index
related things as that's how I got started with the git project, I
couldn't follow all the conversations that were going on there.

*1*: 
https://github.com/gitster/git/blob/52015aaf9d19c97b52c47c7046058e6d029ff856/builtin/am.c#L1844

> > +
> >  int 

Re: [PATCH v2 6/8] travis-ci: don't install 'language-pack-is' package

[Junio C Hamano]

Lars Schneider  writes:

>> It seems we would loose coverage with this patch, so it should be
>> dropped.
>
> Yeah. I think we should add a comment to the travis.yml to avoid
> future confusion. I'll do it unless you beat me to it with a re-roll.

Rather, it should be commented close to where is locale is used in
tests, and possibly in t/README, I would think.  Those who want to
use t/*, not necessarily using Travis, would benefit from the hint,
"installing icelandic locale may give you better test coverage", or
something like that.

Re: [PATCH v3] imap-send: URI encode server folder

[Junio C Hamano]

Kaartic Sivaraam  writes:

> From: Nicolas Morey-Chaisemartin 
>
> When trying to send a patch using 'imap-send' with 'curl' and the
> following configuration:
>
> [imap]
>   folder = "[Gmail]/Drafts"
>   host = imaps://imap.gmail.com
>   port = 993
>   sslverify = false
>
> results in the following error,
>
> curl_easy_perform() failed: URL using bad/illegal format or missing URL
>
> This is a consequence of not URI-encoding the folder portion of
> the URL which contains characters such as '[' which are not
> allowed in a URI. According to RFC3986, these characters should be
> URI-encoded.
>
> So, URI-encode the folder before adding it to the URI to ensure it doesn't
> contain characters that aren't allowed in a URI.
>
> Reported-by: Doron Behar 
> Signed-off-by: Nicolas Morey-Chaisemartin 
> Signed-off-by: Kaartic Sivaraam 
> ---
> Changes in v3:
> - updated commit message as suggested by Eric (convert past tense
>   to present tense) and added a few tweaks to it that striked me
>
> Eric Sunshine  writes:
>> Thanks for taking up the slack.
>
> You're welcome. It was easier than waiting for this patch to be
> updated so it could get into 'pu' ;-)

Looks good.  Thanks.


>  imap-send.c | 8 +++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/imap-send.c b/imap-send.c
> index 54e6a80fd..36c7c1b4f 100644
> --- a/imap-send.c
> +++ b/imap-send.c
> @@ -1412,6 +1412,7 @@ static CURL *setup_curl(struct imap_server_conf *srvc, 
> struct credential *cred)
>  {
>   CURL *curl;
>   struct strbuf path = STRBUF_INIT;
> + char *uri_encoded_folder;
>  
>   if (curl_global_init(CURL_GLOBAL_ALL) != CURLE_OK)
>   die("curl_global_init failed");
> @@ -1429,7 +1430,12 @@ static CURL *setup_curl(struct imap_server_conf *srvc, 
> struct credential *cred)
>   strbuf_addstr(, server.host);
>   if (!path.len || path.buf[path.len - 1] != '/')
>   strbuf_addch(, '/');
> - strbuf_addstr(, server.folder);
> +
> + uri_encoded_folder = curl_easy_escape(curl, server.folder, 0);
> + if (!uri_encoded_folder)
> + die("failed to encode server folder");
> + strbuf_addstr(, uri_encoded_folder);
> + curl_free(uri_encoded_folder);
>  
>   curl_easy_setopt(curl, CURLOPT_URL, path.buf);
>   strbuf_release();

Re: [PATCH v3] imap-send: URI encode server folder

[Jonathan Nieder]

Kaartic Sivaraam wrote:

> From: Nicolas Morey-Chaisemartin 
>
> When trying to send a patch using 'imap-send' with 'curl' and the
> following configuration:
>
> [imap]
>   folder = "[Gmail]/Drafts"
>   host = imaps://imap.gmail.com
>   port = 993
>   sslverify = false
>
> results in the following error,
>
> curl_easy_perform() failed: URL using bad/illegal format or missing URL
>
> This is a consequence of not URI-encoding the folder portion of
> the URL which contains characters such as '[' which are not
> allowed in a URI. According to RFC3986, these characters should be
> URI-encoded.
>
> So, URI-encode the folder before adding it to the URI to ensure it doesn't
> contain characters that aren't allowed in a URI.
>
> Reported-by: Doron Behar 
> Signed-off-by: Nicolas Morey-Chaisemartin 
> Signed-off-by: Kaartic Sivaraam 
> ---
>  imap-send.c | 8 +++-
>  1 file changed, 7 insertions(+), 1 deletion(-)

Thanks!
Reviewed-by: Jonathan Nieder 

Is there a straightforward way to check that this behavior gets
preserved in tests?

Sincerely,
Jonathan

Re: [PATCH v2 6/8] travis-ci: don't install 'language-pack-is' package

[Lars Schneider]

> On 18 Dec 2017, at 23:04, SZEDER Gábor  wrote:
> 
> On Mon, Dec 18, 2017 at 10:33 PM, Lars Schneider
>  wrote:
>> 
>>> On 16 Dec 2017, at 13:57, SZEDER Gábor  wrote:
>>> 
>>> Ever since we have started to use Travis CI in 522354d70 (Add Travis
>>> CI support, 2015-11-27), our 64 bit Linux build jobs install the
>>> 'languate-pack-is' package.  That commit doesn't discuss why it was
>>> deemed necessary back then, but Travis CI can build and test Git
>>> without that package just fine, even that commit introducing Travis CI
>>> support.
>> 
>> If I remember correctly then we had to install the Icelandic
>> language pack for the i18n tests (e.g. t0204):
>> 
>> https://github.com/git/git/blob/master/t/lib-gettext.sh
>> https://packages.ubuntu.com/trusty/language-pack-is
>> 
>> However, I checked your Travis-CI and I cannot spot any errors:
>> https://travis-ci.org/szeder/git/jobs/317494789
> 
> Ah, now I start to understand.  I was only looking for errors, too, but
> there are none, because the tests requiring the language pack are
> protected by the appropriate prerequisites.  On my box, without and
> with the language pack:
> 
>  $ ./t0204-gettext-reencode-sanity.sh
>  # lib-gettext: No is_IS UTF-8 locale available
>  # lib-gettext: No is_IS ISO-8859-1 locale available
>  ok 1 # skip gettext: Emitting UTF-8 from our UTF-8 *.mo files /
> Icelandic (missing GETTEXT_LOCALE)
>  ok 2 # skip gettext: Emitting UTF-8 from our UTF-8 *.mo files /
> Runes (missing GETTEXT_LOCALE)
>  ok 3 # skip gettext: Emitting ISO-8859-1 from our UTF-8 *.mo files /
> Icelandic (missing GETTEXT_ISO_LOCALE)
>  ok 4 # skip gettext: impossible ISO-8859-1 output (missing 
> GETTEXT_ISO_LOCALE)
>  ok 5 # skip gettext: Fetching a UTF-8 msgid -> UTF-8 (missing GETTEXT_LOCALE)
>  ok 6 # skip gettext: Fetching a UTF-8 msgid -> ISO-8859-1 (missing
> GETTEXT_ISO_LOCALE)
>  ok 7 # skip gettext.c: git init UTF-8 -> UTF-8 (missing GETTEXT_LOCALE)
>  ok 8 # skip gettext.c: git init UTF-8 -> ISO-8859-1 (missing
> GETTEXT_ISO_LOCALE)
>  # passed all 8 test(s)
> 
>  $ sudo apt-get install language-pack-is
>  [...]
>  $ ./t0204-gettext-reencode-sanity.sh
>  # lib-gettext: Found 'is_IS.utf8' as an is_IS UTF-8 locale
>  # lib-gettext: No is_IS ISO-8859-1 locale available
>  ok 1 - gettext: Emitting UTF-8 from our UTF-8 *.mo files / Icelandic
>  ok 2 - gettext: Emitting UTF-8 from our UTF-8 *.mo files / Runes
>  ok 3 # skip gettext: Emitting ISO-8859-1 from our UTF-8 *.mo files /
> Icelandic (missing GETTEXT_ISO_LOCALE)
>  ok 4 # skip gettext: impossible ISO-8859-1 output (missing 
> GETTEXT_ISO_LOCALE)
>  ok 5 - gettext: Fetching a UTF-8 msgid -> UTF-8
>  ok 6 # skip gettext: Fetching a UTF-8 msgid -> ISO-8859-1 (missing
> GETTEXT_ISO_LOCALE)
>  ok 7 - gettext.c: git init UTF-8 -> UTF-8
>  ok 8 # skip gettext.c: git init UTF-8 -> ISO-8859-1 (missing
> GETTEXT_ISO_LOCALE)
>  # passed all 8 test(s)
>  1..8
> 
> I'd expect something like this in the Travis CI build jobs as well, but
> prove hides the detailed output.

Ahh. That explains it!


> It seems we would loose coverage with this patch, so it should be
> dropped.

Yeah. I think we should add a comment to the travis.yml to avoid
future confusion. I'll do it unless you beat me to it with a re-roll.


- Lars

> 
>> I am a bit puzzled. Maybe the Icelandic language pack was not part
>> of the Ubuntu image that was used when we introduced Travis CI?
>> 
>> The Ubuntu default image was 12.04 back then:
>> https://travis-ci.org/git/git/jobs/100241871
>> 
>> Nowadays it is 14.04.
>> 
>> @Avar:
>> Do you know what kind of errors we should expect if the language
>> pack is not installed?
>> 
>> Thanks,
>> Lars
>> 
>>> 
>>> Signed-off-by: SZEDER Gábor 
>>> ---
>>> .travis.yml | 1 -
>>> 1 file changed, 1 deletion(-)
>>> 
>>> diff --git a/.travis.yml b/.travis.yml
>>> index 4684b3f4f..ea11b5af6 100644
>>> --- a/.travis.yml
>>> +++ b/.travis.yml
>>> @@ -17,7 +17,6 @@ compiler:
>>> addons:
>>>  apt:
>>>packages:
>>> -- language-pack-is
>>>- git-svn
>>>- apache2
>>> 
>>> --
>>> 2.15.1.429.ga000dd9c7

[ANNOUNCE] tig-2.3.2

[Jonas Fonseca]

Hello,

A regression in 2.3.1 related with the detection of busy loops has
been fixed in version 2.3.2.

Release notes
-
Bug fixes:

 - Fix busy loop detection to handle large repos. (GH #164)

Change summary
--
The diffstat and log summary for changes made in this release.

 INSTALL.adoc  | 4 ++--
 Makefile  | 2 +-
 NEWS.adoc | 7 +++
 src/display.c | 4 +++-
 tools/aspell.dict | 2 +-
 5 files changed, 14 insertions(+), 5 deletions(-)

Jonas Fonseca (2):
  Only check for busy loop when no views are updating
  tig-2.3.2

-- 
Jonas Fonseca

Re: [PATCH v2 6/8] travis-ci: don't install 'language-pack-is' package

[SZEDER Gábor]

On Mon, Dec 18, 2017 at 10:33 PM, Lars Schneider
 wrote:
>
>> On 16 Dec 2017, at 13:57, SZEDER Gábor  wrote:
>>
>> Ever since we have started to use Travis CI in 522354d70 (Add Travis
>> CI support, 2015-11-27), our 64 bit Linux build jobs install the
>> 'languate-pack-is' package.  That commit doesn't discuss why it was
>> deemed necessary back then, but Travis CI can build and test Git
>> without that package just fine, even that commit introducing Travis CI
>> support.
>
> If I remember correctly then we had to install the Icelandic
> language pack for the i18n tests (e.g. t0204):
>
> https://github.com/git/git/blob/master/t/lib-gettext.sh
> https://packages.ubuntu.com/trusty/language-pack-is
>
> However, I checked your Travis-CI and I cannot spot any errors:
> https://travis-ci.org/szeder/git/jobs/317494789

Ah, now I start to understand.  I was only looking for errors, too, but
there are none, because the tests requiring the language pack are
protected by the appropriate prerequisites.  On my box, without and
with the language pack:

  $ ./t0204-gettext-reencode-sanity.sh
  # lib-gettext: No is_IS UTF-8 locale available
  # lib-gettext: No is_IS ISO-8859-1 locale available
  ok 1 # skip gettext: Emitting UTF-8 from our UTF-8 *.mo files /
Icelandic (missing GETTEXT_LOCALE)
  ok 2 # skip gettext: Emitting UTF-8 from our UTF-8 *.mo files /
Runes (missing GETTEXT_LOCALE)
  ok 3 # skip gettext: Emitting ISO-8859-1 from our UTF-8 *.mo files /
Icelandic (missing GETTEXT_ISO_LOCALE)
  ok 4 # skip gettext: impossible ISO-8859-1 output (missing GETTEXT_ISO_LOCALE)
  ok 5 # skip gettext: Fetching a UTF-8 msgid -> UTF-8 (missing GETTEXT_LOCALE)
  ok 6 # skip gettext: Fetching a UTF-8 msgid -> ISO-8859-1 (missing
GETTEXT_ISO_LOCALE)
  ok 7 # skip gettext.c: git init UTF-8 -> UTF-8 (missing GETTEXT_LOCALE)
  ok 8 # skip gettext.c: git init UTF-8 -> ISO-8859-1 (missing
GETTEXT_ISO_LOCALE)
  # passed all 8 test(s)

  $ sudo apt-get install language-pack-is
  [...]
  $ ./t0204-gettext-reencode-sanity.sh
  # lib-gettext: Found 'is_IS.utf8' as an is_IS UTF-8 locale
  # lib-gettext: No is_IS ISO-8859-1 locale available
  ok 1 - gettext: Emitting UTF-8 from our UTF-8 *.mo files / Icelandic
  ok 2 - gettext: Emitting UTF-8 from our UTF-8 *.mo files / Runes
  ok 3 # skip gettext: Emitting ISO-8859-1 from our UTF-8 *.mo files /
Icelandic (missing GETTEXT_ISO_LOCALE)
  ok 4 # skip gettext: impossible ISO-8859-1 output (missing GETTEXT_ISO_LOCALE)
  ok 5 - gettext: Fetching a UTF-8 msgid -> UTF-8
  ok 6 # skip gettext: Fetching a UTF-8 msgid -> ISO-8859-1 (missing
GETTEXT_ISO_LOCALE)
  ok 7 - gettext.c: git init UTF-8 -> UTF-8
  ok 8 # skip gettext.c: git init UTF-8 -> ISO-8859-1 (missing
GETTEXT_ISO_LOCALE)
  # passed all 8 test(s)
  1..8

I'd expect something like this in the Travis CI build jobs as well, but
prove hides the detailed output.

It seems we would loose coverage with this patch, so it should be
dropped.

> I am a bit puzzled. Maybe the Icelandic language pack was not part
> of the Ubuntu image that was used when we introduced Travis CI?
>
> The Ubuntu default image was 12.04 back then:
> https://travis-ci.org/git/git/jobs/100241871
>
> Nowadays it is 14.04.
>
> @Avar:
> Do you know what kind of errors we should expect if the language
> pack is not installed?
>
> Thanks,
> Lars
>
>>
>> Signed-off-by: SZEDER Gábor 
>> ---
>> .travis.yml | 1 -
>> 1 file changed, 1 deletion(-)
>>
>> diff --git a/.travis.yml b/.travis.yml
>> index 4684b3f4f..ea11b5af6 100644
>> --- a/.travis.yml
>> +++ b/.travis.yml
>> @@ -17,7 +17,6 @@ compiler:
>> addons:
>>   apt:
>> packages:
>> -- language-pack-is
>> - git-svn
>> - apache2
>>
>> --
>> 2.15.1.429.ga000dd9c7
>>
>

Re: [PATCH v1] convert: add support for 'encoding' attribute

[Johannes Sixt]

Am 18.12.2017 um 11:13 schrieb Torsten Bögershausen:

Just to confirm my missing knowledge here:
Does this mean, that git-gui and gitk can decode/reencode
the content of a file/blob, when the .gitattributes say so ?


No. I think they parse the output of git-diff et.al., split it per file, 
and then consult .gitattributes to interpret the byte sequence according 
to the encoding.


-- Hannes

Re: [PATCH v2 1/8] travis-ci: use 'set -x' in select 'ci/*' scripts for extra tracing

[Lars Schneider]

> On 16 Dec 2017, at 13:54, SZEDER Gábor  wrote:
> 
> While the build logic was embedded in our '.travis.yml', Travis CI
> used to produce a nice trace log including all commands executed in
> those embedded scriptlets.  Since 657343a60 (travis-ci: move Travis CI
> code into dedicated scripts, 2017-09-10), however, we only see the
> name of the dedicated scripts, but not what those scripts are actually
> doing, resulting in a less useful trace log about e.g. installing
> dependencies.  A patch later in this series will move setting
> environment variables from '.travis.yml' to 'ci/lib-travisci.sh', so
> not even those will be included in the trace log.  Unrelated to
> 657343a60, 'ci/test-documentation.sh' runs a bunch of 'test -s '
> checks which would fail quietly if something were wrong, leaving no
> clue about which one of those checks triggered the failure.
> 
> Use 'set -x' in 'ci/lib-travisci.sh' to get more detailed trace log
> about the commands executed in the 'ci/*' scripts.  Use it in
> 'ci/run-linux32-build.sh' as well, which is run in a Docker container
> and therefore doesn't source 'ci/lib-travisci.sh'.  The secret token
> used for the Windows builds is specified as an encrypted environment
> variable in git/git repository settings on Travis CI and it's redacted
> in the trace logs even with 'set -x'.  However, disable this tracing
> in 'ci/print-test-failures.sh', as it produces far too much noise in
> the output of that script.

I ACK too soon in my other email ;-)

Can you disable the trace log for the loop in "run-windows-build.sh":
https://github.com/git/git/blob/52015aaf9d19c97b52c47c7046058e6d029ff856/ci/run-windows-build.sh#L75-L88

Otherwise the Travis output look like this:
https://travis-ci.org/git/git/jobs/316791071

- Lars

> 
> Signed-off-by: SZEDER Gábor 
> ---
> ci/lib-travisci.sh| 6 --
> ci/print-test-failures.sh | 3 +++
> ci/run-linux32-build.sh   | 2 ++
> 3 files changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/ci/lib-travisci.sh b/ci/lib-travisci.sh
> index ac05f1f46..2d0d1d613 100755
> --- a/ci/lib-travisci.sh
> +++ b/ci/lib-travisci.sh
> @@ -22,8 +22,10 @@ skip_branch_tip_with_tag () {
> }
> 
> # Set 'exit on error' for all CI scripts to let the caller know that
> -# something went wrong
> -set -e
> +# something went wrong.
> +# Set tracing executed commands, primarily setting environment variables
> +# and installing dependencies.
> +set -ex
> 
> skip_branch_tip_with_tag
> 
> diff --git a/ci/print-test-failures.sh b/ci/print-test-failures.sh
> index 8c8973cbf..f757e616c 100755
> --- a/ci/print-test-failures.sh
> +++ b/ci/print-test-failures.sh
> @@ -5,6 +5,9 @@
> 
> . ${0%/*}/lib-travisci.sh
> 
> +# Tracing executed commands would produce too much noise in this script.
> +set +x
> +
> for TEST_EXIT in t/test-results/*.exit
> do
>   if [ "$(cat "$TEST_EXIT")" != "0" ]
> diff --git a/ci/run-linux32-build.sh b/ci/run-linux32-build.sh
> index e30fb2cdd..a8518eddf 100755
> --- a/ci/run-linux32-build.sh
> +++ b/ci/run-linux32-build.sh
> @@ -6,6 +6,8 @@
> #   run-linux32-build.sh [host-user-id]
> #
> 
> +set -x
> +
> # Update packages to the latest available versions
> linux32 --32bit i386 sh -c '
> apt update >/dev/null &&
> -- 
> 2.15.1.429.ga000dd9c7
> 

[FYI PATCH] t/helper/test-lazy-name-hash: fix compilation

[Stefan Beller]

I was compiling origin/master today with stricter compiler flags today
and was greeted by

t/helper/test-lazy-init-name-hash.c: In function ‘cmd_main’:
t/helper/test-lazy-init-name-hash.c:172:5: error: ‘nr_threads_used’ may be used 
uninitialized in this function [-Werror=maybe-uninitialized]
 printf("avg [size %8d] [single %f] %c [multi %f %d]\n",
 ^~~
 nr,
 ~~~
 (double)avg_single/10,
 ~~
 (avg_single < avg_multi ? '<' : '>'),
 ~
 (double)avg_multi/10,
 ~
 nr_threads_used);
 
t/helper/test-lazy-init-name-hash.c:115:6: note: ‘nr_threads_used’ was declared 
here
  int nr_threads_used;
  ^~~

I do not see how we can arrive at that line without having `nr_threads_used`
initialized, as we'd have `count > 1`  (which asserts that we ran the
loop above at least once, such that it *should* be initialized).

I do not have time to dive into further analysis.

Signed-off-by: Stefan Beller 
---
 t/helper/test-lazy-init-name-hash.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/helper/test-lazy-init-name-hash.c 
b/t/helper/test-lazy-init-name-hash.c
index 6368a89345..297fb01d61 100644
--- a/t/helper/test-lazy-init-name-hash.c
+++ b/t/helper/test-lazy-init-name-hash.c
@@ -112,7 +112,7 @@ static void analyze_run(void)
 {
uint64_t t1s, t1m, t2s, t2m;
int cache_nr_limit;
-   int nr_threads_used;
+   int nr_threads_used = 0;
int i;
int nr;
 
-- 
2.15.1.620.gb9897f4670-goog

Re: [PATCH v2 0/8] Travis CI cleanups

[Lars Schneider]

> On 16 Dec 2017, at 13:54, SZEDER Gábor  wrote:
> 
> This is a reroll of 'sg/travis-fixes'.
> 
> Changes since the previous round:
> 
> - Patch 1 got updated following the discussion:
> 
>   - I went with enabling tracing executed commands everywhere,
> including the Windows build job, except where we know it causes way
> too much clutter, which is currently only
> 'ci/print-test-failures.sh'.
> 
>   - Also enable this tracing in 'ci/run-linux32-build.sh', which
> doesn't source 'ci/lib-travisci.sh' as it's run inside a Docker
> container.
> 
>   - The commit message got updated accordingly, including a note about
> the Windows build job's secret token.
> I would like to get an Acked-by: from Dscho on this patch before it
> gets merged.
> 
> - Patches 5-8 are new.  They are various fixes/cleanups unrelated to
>   the Travis CI scriptification, but I don't think it's worth to have
>   them in separate patch series.

This cleanup series looks very good. ACK. Thank you, Gabor!

The is the only potential nit I see:
https://public-inbox.org/git/8f53ef33-6fda-484c-91a4-49cf24c0b...@gmail.com/

- Lars

> 
> SZEDER Gábor (8):
>  travis-ci: use 'set -x' in select 'ci/*' scripts for extra tracing
>  travis-ci: introduce a $jobname variable for 'ci/*' scripts
>  travis-ci: move setting environment variables to 'ci/lib-travisci.sh'
>  travis-ci: set GIT_TEST_HTTPD in 'ci/lib-travisci.sh'
>  travis-ci: don't install default addon packages for the 32 bit Linux
>build
>  travis-ci: don't install 'language-pack-is' package
>  travis-ci: save prove state for the 32 bit Linux build
>  travis-ci: only print test failures if there are test results
>available
> 
> .travis.yml| 28 ++--
> ci/install-dependencies.sh |  8 +++-
> ci/lib-travisci.sh | 38 ++
> ci/print-test-failures.sh  |  9 +
> ci/run-linux32-build.sh|  3 +++
> ci/run-linux32-docker.sh   |  1 +
> 6 files changed, 56 insertions(+), 31 deletions(-)
> 
> -- 
> 2.15.1.429.ga000dd9c7
> 

Re: commit-msg hook possible bad behavior with -F

[Seth Raymond]

I see that line of thinking, and agree that it makes sense. I'm having
problems with an IDE (Qt Creator) and I think I know where the problem
lies now. I believe Qt Creator doesn't actually call git commit until
after the user generates a message in the message box, meaning my
prepare-commit-msg hook is run after I've already written a message
out to .

I don't think there's a fix to it due to the IDE's seemingly-poor
implementation of the git plugin. Thanks for the clarification!

On Mon, Dec 18, 2017 at 1:31 PM, Eric Sunshine  wrote:
> On Mon, Dec 18, 2017 at 11:43 AM, Seth Raymond  
> wrote:
>> If a commit is invoked with -F , indicating that the commit
>> message should be read from an existing file, the the
>> prepare-commit-msg and commit-msg hooks do not operate on . The
>> first argument to the hook is always /COMMIT_EDITMSG, rather
>> than .
>>
>> Am I wrong in this line of thinking?
>
> The content of  gets copied into COMMIT_EDITMSG, so the hook
> does see the supplied message, as expected. Given that the hook is
> allowed to edit the message, it makes sense that it works on
> COMMIT_EDITMSG rather than on  directly.

Re: [PATCH v2 6/8] travis-ci: don't install 'language-pack-is' package

[Lars Schneider]

> On 16 Dec 2017, at 13:57, SZEDER Gábor  wrote:
> 
> Ever since we have started to use Travis CI in 522354d70 (Add Travis
> CI support, 2015-11-27), our 64 bit Linux build jobs install the
> 'languate-pack-is' package.  That commit doesn't discuss why it was
> deemed necessary back then, but Travis CI can build and test Git
> without that package just fine, even that commit introducing Travis CI
> support.

If I remember correctly then we had to install the Icelandic
language pack for the i18n tests (e.g. t0204):

https://github.com/git/git/blob/master/t/lib-gettext.sh
https://packages.ubuntu.com/trusty/language-pack-is

However, I checked your Travis-CI and I cannot spot any errors:
https://travis-ci.org/szeder/git/jobs/317494789

I am a bit puzzled. Maybe the Icelandic language pack was not part
of the Ubuntu image that was used when we introduced Travis CI?

The Ubuntu default image was 12.04 back then:
https://travis-ci.org/git/git/jobs/100241871

Nowadays it is 14.04.

@Avar: 
Do you know what kind of errors we should expect if the language 
pack is not installed?

Thanks,
Lars

> 
> Signed-off-by: SZEDER Gábor 
> ---
> .travis.yml | 1 -
> 1 file changed, 1 deletion(-)
> 
> diff --git a/.travis.yml b/.travis.yml
> index 4684b3f4f..ea11b5af6 100644
> --- a/.travis.yml
> +++ b/.travis.yml
> @@ -17,7 +17,6 @@ compiler:
> addons:
>   apt:
> packages:
> -- language-pack-is
> - git-svn
> - apache2
> 
> -- 
> 2.15.1.429.ga000dd9c7
> 

Did you get my message this time?

[Friedrich Mayrhofer]

-- 

This is the second time.

I, Friedrich Mayrhofer Donate $ 1,000,000.00 to You, Email Me personally for 
more details.

Regards. 
Friedrich Mayrhofer

Re: Q: rational for $XDG_CONFIG_HOME/git/config to be "non global" or just a bug?

[Jacob Keller]

On Sun, Dec 17, 2017 at 10:40 PM, Jeff King  wrote:
> On Sun, Dec 17, 2017 at 08:03:41PM -0800, Jacob Keller wrote:
>
>> I do find it a bit weird that --global writes to one of either file,
>> and doesn't read from both. I'd rather have --global "only" be
>> .gitconfig, and instead add a new option for handling XDG file, and
>> then have it such that it reads them in system -> xdg ->
>> home/.gitconfig -> local, which allows for local .gitconfig to
>> override XDG config, but logically treat them just like we do any
>> other files.
>
> I find it weird, too, but I'm not sure that's the right direction. It
> means that users have to start caring about using "--xdg" instead of
> "--global" if that's what they want to write to. The original idea was
> that the transition to xdg should be fairly seamless, and that --global
> would be an abstraction over both.
>
> To complete that abstraction it seems like reading via "--global" should
> read from both (in the same precedence order that normal config lookup
> uses). If you only use one, there wouldn't be any change in behavior.
> And if you use both, then the behavior makes sense as a subset of the
> normal config lookup. I.e., it could even be explained as:
>
>   If you give no "source", normal config lookup is similar to checking
>   "--system", then "--global", then "--local".
>
> The only person who might be affected is somebody who carries both files
> _and_ really wanted "--global" to read from one specific file (though I
> have no idea from which without looking at the source, and from reading
> this thread it seems I am not the only one who would be confused). So
> I'd be OK calling that an unintended and unsupported behavior, and the
> right thing all along should have been to use "--file=" if you really
> want to avoid "--global" automagic.
>
> -Peff

I think this end game is fine with me too, it's definitely better than
what we have now.

Thanks,
Jake

Re: [PATCH] fmt-merge-msg: avoid leaking strbuf in shortlog()

[René Scharfe]

Am 08.12.2017 um 22:28 schrieb Jeff King:
> On Fri, Dec 08, 2017 at 10:37:08AM -0800, Junio C Hamano wrote:
> 
>>> The two modes (dup/nodup) make string_list code tricky.  Not sure
>>> how far we'd get with something simpler (e.g. an array of char pointers),
>>> but having the caller do all string allocations would make the code
>>> easier to analyze.
>>
>> Yes.
>>
>> It probably would have been more sensible if the API did not have
>> two modes (instead, have the caller pass whatever string to be
>> stored, *and* make the caller responsible for freeing them *if* it
>> passed an allocated string).
> 
> I'd actually argue the other way: the simplest interface is one where
> the string list owns all of its pointers. That keeps the
> ownership/lifetime issues clear, and it's one less step for the caller
> to have to remember to do at the end (they do have to clear() the list,
> but they must do that anyway to free the array of items).
> 
> It does mean that some callers may have to remember to free a temporary
> buffer right after adding its contents to the list. But that's a lesser
> evil, I think, since the memory ownership issues are all clearly
> resolved at the time of add.
> 
> The big cost is just extra copies/allocations.

An interface requiring callers to allocate can be used to implement a
wrapper that does all allocations for them -- the other way around is
harder.  It can be used to avoid object duplication, but duplicates
functions.  No idea if that's worth it.
 
>> For the push_refs_with_push() patch you sent, another possible fix
>> would be to make cas_options a nodup kind so that the result of
>> strbuf_detach() does not get an extra strdup to be lost when placed
>> in cas_options.  With the current string-list API that would not
>> quite work, because freeing done in _release() is tied to the
>> "dup/nodup" ness of the string list.  I think there even is a
>> codepath that initializes a string_list as nodup kind, stuffs string
>> in it giving the ownership, and then flips it into dup kind just
>> before calling _release() only to have it free the strings, or
>> something silly/ugly like that.
> 
> Yes, the first grep hit for NODUP is bisect_clean_state(), which does
> this. I think it would be more clear if we could do:
> 
> diff --git a/bisect.c b/bisect.c
> index 0fca17c02b..7c59408a13 100644
> --- a/bisect.c
> +++ b/bisect.c
> @@ -1060,8 +1060,7 @@ static int mark_for_removal(const char *refname, const 
> struct object_id *oid,
>   int flag, void *cb_data)
>   {
>   struct string_list *refs = cb_data;
> - char *ref = xstrfmt("refs/bisect%s", refname);
> - string_list_append(refs, ref);
> + string_list_appendf(refs, "refs/bisect%s", refname);
>   return 0;
>   }
>   
> @@ -1070,11 +1069,10 @@ int bisect_clean_state(void)
>   int result = 0;
>   
>   /* There may be some refs packed during bisection */
> - struct string_list refs_for_removal = STRING_LIST_INIT_NODUP;
> + struct string_list refs_for_removal = STRING_LIST_INIT_DUP;
>   for_each_ref_in("refs/bisect", mark_for_removal, (void *) 
> _for_removal);
>   string_list_append(_for_removal, xstrdup("BISECT_HEAD"));

The xstrdup() here would have to go.

>   result = delete_refs("bisect: remove", _for_removal, REF_NO_DEREF);
> - refs_for_removal.strdup_strings = 1;
>   string_list_clear(_for_removal, 0);
>   unlink_or_warn(git_path_bisect_expected_rev());
>   unlink_or_warn(git_path_bisect_ancestors_ok());
> 
> 
> Having a "format into a string" wrapper doesn't cover _every_ string you
> might want to add to a list, but my experience with argv_array_pushf
> leads me to believe that it covers quite a lot of cases.

It would fit in with the rest of the API -- we have string_list_append()
as a wrapper for string_list_append_nodup()+xstrdup() already.  We also
have similar functions for strbuf and argv_array.  I find it a bit sad
to reimplement xstrfmt() yet again instead of using it directly, though.

René

Re: [PATCH] revision: introduce prepare_revision_walk_extended()

[René Scharfe]

Am 18.12.2017 um 16:10 schrieb Jeff King:
> On Sat, Dec 16, 2017 at 01:12:16PM +0100, René Scharfe wrote:
> 
>> prepare_revision_walk() allows callers to take ownership of the array of
>> pending objects by setting the rev_info flag "leak_pending" and copying
>> the object_array "pending".  They use it to clear commit marks after
>> setup is done.  This interface is brittle enough that it requires
>> extensive comments.
>>
>> Provide an easier way by adding a function that can hand over the array
>> to a caller-supplied output parameter and converting all users of the
>> flag "leak_pending" to call prepare_revision_walk_extended() instead.
> 
> I think this is _better_, but it's still kind of a funny interface.
> 
> The root of the matter is that the revision-walking code doesn't clean
> up after itself. In every case, the caller is just saving these to clean
> up commit marks, isn't it?

bundle also checks if the pending objects exists.

> Could we instead have an interface like:
> 
>revs.clear_commit_marks = 1;
>prepare_revision_walk();
>...
>finish_revision_walk();
> 
> where that final function would do any cleanup, including clearing the
> commit marks. I suspect there are other small bits that get leaked
> because there's not really any "destructor" for a revision walk.
> 
> It's not as flexible as this whole "make a copy of the pending tips"
> thing, but it keeps all of the details abstracted away from the callers.
> 
> Alternatively:
> 
>> +`prepare_revision_walk_extended`::
>> +
>> +Like prepare_revision_walk(), but allows callers to take ownership
>> +of the array of pending objects by passing an object_array pointer
>> +as the second parameter; passing NULL clears the array.
> 
> What if we just got rid of this function and had callers do:
> 
>object_array_copy(_pending, );
>prepare_revision_walk();
>...
>clear_commit_marks_for_object_array(_pending);
> 
> That sidesteps all of the memory ownership issues by just creating a
> copy. That's less efficient, but I'd be surprised if it matters in
> practice (we tend to do one or two revisions per process, there don't
> tend to be a lot of pending tips, and we're really just talking about
> copying some pointers here).

This was done before I added the leak_pending flag.

t5502 and t5571 have test cases with ca. 1000 pending objects, t5551
and t5541 with ca. 2000, p5310 more than 8000.  That's just a few KB.

I don't know if there can be real-world use cases with millions of
entries (when it would start to hurt).

Why does prepare_revision_walk() clear the list of pending objects at
all?  Assuming the list is append-only then perhaps remembering the
last handled index would suffice.

René

[PATCH v3] imap-send: URI encode server folder

[Kaartic Sivaraam]

From: Nicolas Morey-Chaisemartin 

When trying to send a patch using 'imap-send' with 'curl' and the
following configuration:

[imap]
folder = "[Gmail]/Drafts"
host = imaps://imap.gmail.com
port = 993
sslverify = false

results in the following error,

curl_easy_perform() failed: URL using bad/illegal format or missing URL

This is a consequence of not URI-encoding the folder portion of
the URL which contains characters such as '[' which are not
allowed in a URI. According to RFC3986, these characters should be
URI-encoded.

So, URI-encode the folder before adding it to the URI to ensure it doesn't
contain characters that aren't allowed in a URI.

Reported-by: Doron Behar 
Signed-off-by: Nicolas Morey-Chaisemartin 
Signed-off-by: Kaartic Sivaraam 
---
Changes in v3:
- updated commit message as suggested by Eric (convert past tense
  to present tense) and added a few tweaks to it that striked me

Eric Sunshine  writes:
> Thanks for taking up the slack.

You're welcome. It was easier than waiting for this patch to be
updated so it could get into 'pu' ;-)


 imap-send.c | 8 +++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/imap-send.c b/imap-send.c
index 54e6a80fd..36c7c1b4f 100644
--- a/imap-send.c
+++ b/imap-send.c
@@ -1412,6 +1412,7 @@ static CURL *setup_curl(struct imap_server_conf *srvc, 
struct credential *cred)
 {
CURL *curl;
struct strbuf path = STRBUF_INIT;
+   char *uri_encoded_folder;
 
if (curl_global_init(CURL_GLOBAL_ALL) != CURLE_OK)
die("curl_global_init failed");
@@ -1429,7 +1430,12 @@ static CURL *setup_curl(struct imap_server_conf *srvc, 
struct credential *cred)
strbuf_addstr(, server.host);
if (!path.len || path.buf[path.len - 1] != '/')
strbuf_addch(, '/');
-   strbuf_addstr(, server.folder);
+
+   uri_encoded_folder = curl_easy_escape(curl, server.folder, 0);
+   if (!uri_encoded_folder)
+   die("failed to encode server folder");
+   strbuf_addstr(, uri_encoded_folder);
+   curl_free(uri_encoded_folder);
 
curl_easy_setopt(curl, CURLOPT_URL, path.buf);
strbuf_release();
-- 
2.15.1.620.gb9897f467

Re: Usability issue with rebase fork-point option

[Junio C Hamano]

Robert Dailey  writes:

> $ git rebase
> First, rewinding head to replay your work on top of it...
> Applying: Fix state machine hang after integrity checking
>
> Since my merge-base is already the tip of `origin/master`, I expected
> it to say it was up-to-date, as it would if I disabled fork point:
>
> $ git rebase --no-fork-point
> Current branch feature/foo is up to date.

I haven't studied the symptom deeply enough but I have a feeling
that hunk @@ -572,7 +573,7 @@ in 1e0dacdb ("rebase: omit
patch-identical commits with --fork-point", 2014-07-16) may be what
goes wrong.  I do not offhand see why the "already up to date" check
need to be bypassed when fork-point mode is in use, and the log
message of the commit does not explain the reason behind that
change, either.

I wonder what happens if we simply enabled the "avoid unnecessary
rebase if we are already up to date" check even when $restrict_revision
is not empty.

Re: [PATCH v2] imap-send: URI encode server folder

[Eric Sunshine]

On Mon, Dec 18, 2017 at 1:25 PM, Kaartic Sivaraam
 wrote:
> From: Nicolas Morey-Chaisemartin 
> [...]
> resulted in the following error,

s/resulted/results/

> curl_easy_perform() failed: URL using bad/illegal format or missing URL
>
> That was a consequence of the not URI encoding the folder portion of

s/That was/This is/
s/the not/not/
s/URI encoding/URI-encoding/

> the URL which contained characters such as '[' which are not

s/contained/contains/

> allowed in a URI. According to RFC3986, these characters should be
> "URI encoded".
>
> So, URI encode the folder portion of the URL to ensure it doesn't
> contain characters that aren't allowed in a URI.
>
> Reported-by: Doron Behar 
> Signed-off-by: Nicolas Morey-Chaisemartin 
> ---
>
> I came across the same issue that lead to this patch recently and found
> that this patch didn't make it in. So, I thought I could help out and
> hence this v2.

Thanks for taking up the slack.

Re: 'git format-patch --summary' seems to be turning off the stat

[Kaartic Sivaraam]

On Tuesday 19 December 2017 12:07 AM, Junio C Hamano wrote:

Kaartic Sivaraam  writes:


Note: I do see that "--summary" is a diff-option but does that mean we
should't be printing stat information in the patch when the user
didn't mention "--no-stat"? Yeah, "git format-patch  --summary
--stat" does bring back the stat. --- Kaartic


The old design decision shared across "log" family of commands [*1*]
wrt to these "how and what are shown" options is that they started
without any default set of options and the user gave all options (if
they want both summary and stat, they gave them from the command
line); over time they gained a set of built-in default options that
take effect only when the user gives no option.

So, yes, giving "--summary" will turn off the built-in ones that are
"--summary --stat" (IIRC).



Is it worth documenting that built-in defaults are turned off when user 
gives other options? Not sure where, though?



--
Kaartic

Re: Fetching commit instead of ref

[Junio C Hamano]

"Carlsson, Magnus"  writes:

> > So far so good, but then an error message appear:
> error: Server does not allow request for unadvertised object 
> 50f730db793e0733b159326c5a3e78fd48cedfec
> > And nothing seems to be fetched.

Yes, that is what the error message is telling you.  

You'd need to coordinate with the server operator so that the server
allows such an request; uploadpack.allowAnySHA1InWant may help.

Re: 'git format-patch --summary' seems to be turning off the stat

[Junio C Hamano]

Kaartic Sivaraam  writes:

> Note: I do see that "--summary" is a diff-option but does that mean we
> should't be printing stat information in the patch when the user
> didn't mention "--no-stat"? Yeah, "git format-patch  --summary
> --stat" does bring back the stat. --- Kaartic

The old design decision shared across "log" family of commands [*1*]
wrt to these "how and what are shown" options is that they started
without any default set of options and the user gave all options (if
they want both summary and stat, they gave them from the command
line); over time they gained a set of built-in default options that
take effect only when the user gives no option.

So, yes, giving "--summary" will turn off the built-in ones that are
"--summary --stat" (IIRC).


[Footnote]

*1* "show" turns "-p" on by default, but with "show --stat" you can
get "--stat" without the patch.  That comes from the same design.

Re: commit-msg hook possible bad behavior with -F

[Eric Sunshine]

On Mon, Dec 18, 2017 at 11:43 AM, Seth Raymond  wrote:
> If a commit is invoked with -F , indicating that the commit
> message should be read from an existing file, the the
> prepare-commit-msg and commit-msg hooks do not operate on . The
> first argument to the hook is always /COMMIT_EDITMSG, rather
> than .
>
> Am I wrong in this line of thinking?

The content of  gets copied into COMMIT_EDITMSG, so the hook
does see the supplied message, as expected. Given that the hook is
allowed to edit the message, it makes sense that it works on
COMMIT_EDITMSG rather than on  directly.

[PATCH v2] imap-send: URI encode server folder

[Kaartic Sivaraam]

From: Nicolas Morey-Chaisemartin 

When trying to send a patch using 'imap-send' with 'curl' and the
following configuration:

[imap]
folder = "[Gmail]/Drafts"
host = imaps://imap.gmail.com
port = 993
sslverify = false

resulted in the following error,

curl_easy_perform() failed: URL using bad/illegal format or missing URL

That was a consequence of the not URI encoding the folder portion of
the URL which contained characters such as '[' which are not
allowed in a URI. According to RFC3986, these characters should be
"URI encoded".

So, URI encode the folder portion of the URL to ensure it doesn't
contain characters that aren't allowed in a URI.

Reported-by: Doron Behar 
Signed-off-by: Nicolas Morey-Chaisemartin 
---

I came across the same issue that lead to this patch recently and found
that this patch didn't make it in. So, I thought I could help out and
hence this v2.

Eric Sunshine  writes:
> For someone reading this commit message in the future -- someone who
> didn't follow the email thread which led to this patch -- "this fixes"
> doesn't say much about the actual problem being addressed. Can you
> expand the commit message a bit to make it more self-contained? At
> minimum, perhaps show the error message you were experiencing, and
> cite (as Daniel pointed out) RFC 3986 and the bit about a "legal" URL
> not containing brackets.

I guess I covered this part.


> Also, a natural question which pops into the head of someone reading
> this patch is whether other parts of the URL (host, user, etc.) also
> need to be handled similarly. It's possible that you audited the code
> and determined that they are handled fine already, but the reader of
> the commit message is unable to infer that. Consequently, it might be
> nice to have a sentence about that, as well ("other parts of the URL
> are already encoded, thus are fine" or "other parts of the URL are not
> subject to this problem because ...").

I'm not sure about this one. I guess the host and user don't need encoding
as I suspect they wouldn't contain characters that aren't allowed. I might
be wrong, though. Let me know if I'm missing something.


Thanks,
Kaartic

 imap-send.c | 8 +++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/imap-send.c b/imap-send.c
index 54e6a80fd..36c7c1b4f 100644
--- a/imap-send.c
+++ b/imap-send.c
@@ -1412,6 +1412,7 @@ static CURL *setup_curl(struct imap_server_conf *srvc, 
struct credential *cred)
 {
CURL *curl;
struct strbuf path = STRBUF_INIT;
+   char *uri_encoded_folder;
 
if (curl_global_init(CURL_GLOBAL_ALL) != CURLE_OK)
die("curl_global_init failed");
@@ -1429,7 +1430,12 @@ static CURL *setup_curl(struct imap_server_conf *srvc, 
struct credential *cred)
strbuf_addstr(, server.host);
if (!path.len || path.buf[path.len - 1] != '/')
strbuf_addch(, '/');
-   strbuf_addstr(, server.folder);
+
+   uri_encoded_folder = curl_easy_escape(curl, server.folder, 0);
+   if (!uri_encoded_folder)
+   die("failed to encode server folder");
+   strbuf_addstr(, uri_encoded_folder);
+   curl_free(uri_encoded_folder);
 
curl_easy_setopt(curl, CURLOPT_URL, path.buf);
strbuf_release();
-- 
2.15.1.620.gb9897f467

Re: Apparent bug in 'git stash push ' loses untracked files

[Junio C Hamano]

Thomas Gummerer  writes:

> Ah interesting, what you have below looks good to me indeed, it
> matches what I'd expect it to do and fixes the bug that was reported.
> Thanks! 
>
> I've taken the liberty to take what you have below and turned into a
> proper patch, giving you authorship of it, as you wrote the code for
> the fix.  Hope that's the appropriate credit for you for this patch.

Not so fast.

I know why the updated code works like "--hard -- ", but I
do not quite get what the original was trying to do and how it is
different.  Even with your proposed log message, which describes a
symptom (i.e. "untracked files ... be deleted"), it is unclear why
this deletion was happening in the first place.  Specifically, it is
unclear why that "clean --force -q -d" was in there, and are we
breaking other cases by rewriting this codepath without it?

In any case, instead of the 

ls-files -z -- "$@" | checkout-index -z --force --stdin
diff-index -p HEAD -- "$@" | apply --index -R

sequence, a shorter variant that should work is

add -u -- "$@"
diff-index -p --cached HEAD -- "$@" | apply --index -R

Both of these share the same idea.  

 - The first step is to match what is in the index and what is in
   the working tree (i.e. make "diff-files" silent).  The version
   you have does so by checking out what is in the index to the
   working tree.  The shorter one goes the other way and updates the
   index with what is in the working tree.

 - Once that is done, we ask diff-index what got changed since the
   HEAD in the working tree (or in the index in the updated
   one---after the first step that makes the two match, comparing
   with the working tree and comparing with the index should result
   in the same diff; I have a suspicion that "--cached" is faster,
   but we need to benchmark to pick), and ask apply to get rid of
   all these changes, which includes removal of added files, and
   resurrection of removed files.

I think the original that did 'git reset -- "$@"' upfront lost new
paths from the index (without removing it from the working tree), I
am guessing that it is why "clean" was there to get rid of them, and
if that is the reason, I can understand why the original code was
behaving incorrectly---it would get rid of new files that did not
exist in HEAD correctly, but it also would remove untracked ones,
because after that first 'reset', the code couldn't tell between
them.

And I think that is what we want, i.e. why the original was wrong
and how the new one fixes it, to describe in the log message to
justify this change.

One thing that I didn't think through and you need to verify is if
we need to do anything extra to deal with binary files (in the old
days, we needed --full-index and --binary options to produce and
apply a binary patch; I do not offhand know if that is still the
case) in the final "diff-index piped to apply -R --index" dance.

So I am asking you to fill in quite a lot of gaps that I didn't do
with only the above two-liner ;-)  You should take the authorship
and, if you like, credit me with helped-by: or something.

Thanks.


>
> [...]
>
> --- >8 ---
> From: Junio C Hamano 
> Subject: [PATCH] stash: don't delete untracked files that match pathspec
>
> Currently when 'git stash push -- ' is used, untracked files
> that match the pathspec will be deleted, even though they do not end up
> in a stash anywhere.
>
> Untracked files should be left along by 'git stash push -- '
> unless the --untracked or --all flags are given.  Fix that.
>
> Reported-by: Reid Price 
> Test-by: Thomas Gummerer 
> Signed-off-by: Thomas Gummerer 
> ---
>  git-stash.sh |  5 ++---
>  t/t3903-stash.sh | 16 
>  2 files changed, 18 insertions(+), 3 deletions(-)
>
> diff --git a/git-stash.sh b/git-stash.sh
> index 1114005ce2..a979bfb665 100755
> --- a/git-stash.sh
> +++ b/git-stash.sh
> @@ -322,10 +322,9 @@ push_stash () {
>  
>   if test $# != 0
>   then
> - git reset -q -- "$@"
> - git ls-files -z --modified -- "$@" |
> + git ls-files -z -- "$@" |
>   git checkout-index -z --force --stdin
> - git clean --force -q -d -- "$@"
> + git diff-index -p HEAD -- "$@" | git apply --index -R
>   else
>   git reset --hard -q
>   fi
> diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
> index 39c7f2ebd7..6952a031b2 100755
> --- a/t/t3903-stash.sh
> +++ b/t/t3903-stash.sh
> @@ -1064,4 +1064,20 @@ test_expect_success 'stash -k --  leaves 
> unstaged files intact' '
>   test foo,bar = $(cat foo),$(cat bar)
>  '
>  
> +test_expect_success 'stash --  leaves untracked files in subdir 
> intact' '
> + git reset &&
> + >subdir/untracked &&
> + >subdir/tracked1 &&
> + 

Re: Fetching commit instead of ref

[Stefan Beller]

On Mon, Dec 18, 2017 at 4:30 AM, Carlsson, Magnus
 wrote:
> Hi
>
> I am involved in the git-subrepo project 
> (https://github.com/ingydotnet/git-subrepo/). It's an attempt to simplify the 
> inclusion of repos into other repos.
>
> In a certain situation I would really need to fetch all commits related to a 
> specific commit (SHA). I have read the git fetch documentation and found 
> nothing regarding this. It only seems to support fetching references.
>
> I found some traces on stack overflow:
> https://stackoverflow.com/questions/14872486/retrieve-specific-commit-from-a-remote-git-repository
>
> Following that recommendation it feels like it almost works:
> $ git fetch subrepo 
> 50f730db793e0733b159326c5a3e78fd48cedfec:refs/remote/subrepo/foo-commit
> remote: Counting objects: 2311, done.
> remote: Total 2311 (delta 0), reused 0 (delta 0), pack-reused 2311
> Receiving objects: 100% (2311/2311), 703.64 KiB | 0 bytes/s, done.
> Resolving deltas: 100% (1174/1174), done.
> > So far so good, but then an error message appear:
> error: Server does not allow request for unadvertised object 
> 50f730db793e0733b159326c5a3e78fd48cedfec
> > And nothing seems to be fetched.
>
> Is there a way to fetch a commit and any ancestors to that commit based on a 
> SHA?

Ask the server operator to configure the server to allow fetching commits,
specifically "git config uploadpack.allowReachableSHA1InWant 1"


>
> Why do I need this?
> In git-subrepo we try to recreate another repo within our main repo. Creating 
> the necessary parent references when they appear. In some cases we need to 
> make sure that we have access to the correct commits from the subrepo, but we 
> don't have any references except a SHA.

A very similar issue happens with submodules, which tries to fetch the
branch(es) first and then by SHA1 as a fallback, but this fallback may
fail as well due to the miss-configured server, at that point
submodules just error out.

Re: [PATCH v2 2/3] prune: fix pruning with multiple worktrees and split index

[Brandon Williams]

On 12/17, Thomas Gummerer wrote:
> be489d02d2 ("revision.c: --indexed-objects add objects from all
> worktrees", 2017-08-23) made sure that pruning takes objects from all
> worktrees into account.
> 
> It did that by reading the index of every worktree and adding the
> necessary index objects to the set of pending objects.  The index is
> read by read_index_from.  As mentioned in the previous commit,
> read_index_from depends on the CWD for the location of the split index,

As I mentioned before this doesn't actually depend on the CWD but
rather the per-worktree gitdir.

> and add_index_objects_to_pending doesn't set that before using
> read_index_from.
> 
> Instead of using read_index_from, use repo_read_index, which is aware of
> the proper paths for the worktree.
> 
> This fixes t5304-prune when ran with GIT_TEST_SPLIT_INDEX set.
> 
> Signed-off-by: Thomas Gummerer 
> ---
>  repository.c | 11 +++
>  repository.h |  2 ++
>  revision.c   | 14 +-
>  3 files changed, 22 insertions(+), 5 deletions(-)
> 
> diff --git a/repository.c b/repository.c
> index 928b1f553d..3c9bfbd1b8 100644
> --- a/repository.c
> +++ b/repository.c
> @@ -2,6 +2,7 @@
>  #include "repository.h"
>  #include "config.h"
>  #include "submodule-config.h"
> +#include "worktree.h"
>  
>  /* The main repository */
>  static struct repository the_repo = {
> @@ -146,6 +147,16 @@ int repo_init(struct repository *repo, const char 
> *gitdir, const char *worktree)
>   return -1;
>  }
>  
> +/*
> + * Initialize 'repo' based on the provided worktree
> + * Return 0 upon success and a non-zero value upon failure.
> + */
> +int repo_worktree_init(struct repository *repo, struct worktree *worktree)
> +{
> + return repo_init(repo, get_worktree_git_dir(worktree),
> +  worktree->path);

I still feel very unsettled about this and don't think its a good idea.
get_worktree_git_dir depends implicitly on the global the_repository
object and I would like to avoid relying on it for an initialization
function like this.

> +}
> +
>  /*
>   * Initialize 'submodule' as the submodule given by 'path' in parent 
> repository
>   * 'superproject'.
> diff --git a/repository.h b/repository.h
> index 7f5e24a0a2..2adeb05bf4 100644
> --- a/repository.h
> +++ b/repository.h
> @@ -4,6 +4,7 @@
>  struct config_set;
>  struct index_state;
>  struct submodule_cache;
> +struct worktree;
>  
>  struct repository {
>   /* Environment */
> @@ -87,6 +88,7 @@ extern struct repository *the_repository;
>  extern void repo_set_gitdir(struct repository *repo, const char *path);
>  extern void repo_set_worktree(struct repository *repo, const char *path);
>  extern int repo_init(struct repository *repo, const char *gitdir, const char 
> *worktree);
> +extern int repo_worktree_init(struct repository *repo, struct worktree 
> *worktree);
>  extern int repo_submodule_init(struct repository *submodule,
>  struct repository *superproject,
>  const char *path);
> diff --git a/revision.c b/revision.c
> index e2e691dd5a..34e1e4b799 100644
> --- a/revision.c
> +++ b/revision.c
> @@ -22,6 +22,7 @@
>  #include "packfile.h"
>  #include "worktree.h"
>  #include "argv-array.h"
> +#include "repository.h"
>  
>  volatile show_early_output_fn_t show_early_output;
>  
> @@ -1346,15 +1347,18 @@ void add_index_objects_to_pending(struct rev_info 
> *revs, unsigned int flags)
>   worktrees = get_worktrees(0);
>   for (p = worktrees; *p; p++) {
>   struct worktree *wt = *p;
> - struct index_state istate = { NULL };
> + struct repository *repo;
>  
> + repo = xmalloc(sizeof(struct repository));
>   if (wt->is_current)
>   continue; /* current index already taken care of */
> + if (repo_worktree_init(repo, wt))
> + BUG("couldn't initialize repository object from 
> worktree");
>  
> - if (read_index_from(,
> - worktree_git_path(wt, "index")) > 0)

Ok, after thinking this through a bit more I think a better approach may
be to restructure the call to read_index_from to take in both an index
file as well as the explicit gitdir to use when constructing a path to
the sharedindex file.  That way you can fix this for worktrees and
submodules without having to pass in a repository object to the logic
which is reading an index file as well as avoiding needing to init a
repository object for every worktree.

So you could rework the first patch to do something like:

diff --git a/read-cache.c b/read-cache.c
index 2eb81a66b..3a04b5567 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -1863,20 +1863,19 @@ int do_read_index(struct index_state *istate, const 
char *path, int must_exist)
  * This way, shared index can be removed if they have not been used
  * for some time.
  */
-static void freshen_shared_index(char *base_sha1_hex, int warn)

Re: Fetching commit instead of ref

[Jonathan Tan]

On Mon, 18 Dec 2017 12:30:23 +
"Carlsson, Magnus"  wrote:

> In a certain situation I would really need to fetch all commits
> related to a specific commit (SHA). I have read the git fetch
> documentation and found nothing regarding this. It only seems to
> support fetching references.

The documentation has been updated in version 2.15.0 to describe this.
But as the commit message of commit 83558a412afa ("fetch doc: src side
of refspec could be full SHA-1", 2017-10-18) says, this functionality
was available earlier.

> I found some traces on stack overflow:
> https://stackoverflow.com/questions/14872486/retrieve-specific-commit-from-a-remote-git-repository
> 
> Following that recommendation it feels like it almost works:
> $ git fetch subrepo 
> 50f730db793e0733b159326c5a3e78fd48cedfec:refs/remote/subrepo/foo-commit
> remote: Counting objects: 2311, done.
> remote: Total 2311 (delta 0), reused 0 (delta 0), pack-reused 2311
> Receiving objects: 100% (2311/2311), 703.64 KiB | 0 bytes/s, done.
> Resolving deltas: 100% (1174/1174), done.
> > So far so good, but then an error message appear:
> error: Server does not allow request for unadvertised object 
> 50f730db793e0733b159326c5a3e78fd48cedfec
> > And nothing seems to be fetched.
> 
> Is there a way to fetch a commit and any ancestors to that commit based on a 
> SHA?

You'll need to set uploadpack.allowTipSHA1InWant,
uploadpack.allowReachableSHA1InWant, or uploadpack.allowAnySHA1InWant on
the server. (This might need to be better documented - I see this
documented for fetch-pack, but not fetch.)

Re: [PATCH v2 3/3] travis: run tests with GIT_TEST_SPLIT_INDEX

[Lars Schneider]

> On 17 Dec 2017, at 23:51, Thomas Gummerer  wrote:
> 
> Split index mode only has a few dedicated tests, but as the index is
> involved in nearly every git operation, this doesn't quite cover all the
> ways repositories with split index can break.  To use split index mode
> throughout the test suite a GIT_TEST_SPLIT_INDEX environment variable
> can be set, which makes git split the index at random and thus
> excercises the functionality much more thoroughly.
> 
> As this is not turned on by default, it is not executed nearly as often
> as the test suite is run, so occationally breakages slip through.  Try
> to counteract that by running the test suite with GIT_TEST_SPLIT_INDEX
> mode turned on on travis.
> 
> To avoid using too many cycles on travis only run split index mode in
> the linux-gcc and the linux 32-bit gcc targets.

I am surprised to see the split index mode test for the linux 32-bit
target. Is it likely that a split index bug appears only on 32-bit? 
Wouldn't the linux-gcc target be sufficient to save resources/time?


>  The Linux builds were
> chosen over the Mac OS builds because they tend to be much faster to
> complete.
> 
> The linux gcc build was chosen over the linux clang build because the
> linux clang build is the fastest build, so it can serve as an early
> indicator if something is broken and we want to avoid spending the extra
> cycles of running the test suite twice for that.
> 
> Helped-by: Lars Schneider 
> Helped-by: Junio C Hamano 
> Signed-off-by: Thomas Gummerer 
> ---
> ci/run-linux32-build.sh | 1 +
> ci/run-tests.sh | 4 
> 2 files changed, 5 insertions(+)
> 
> diff --git a/ci/run-linux32-build.sh b/ci/run-linux32-build.sh
> index e30fb2cddc..f173c9cf2a 100755
> --- a/ci/run-linux32-build.sh
> +++ b/ci/run-linux32-build.sh
> @@ -27,4 +27,5 @@ linux32 --32bit i386 su -m -l $CI_USER -c '
> cd /usr/src/git &&
> make --jobs=2 &&
> make --quiet test
> +GIT_TEST_SPLIT_INDEX=YesPlease make --quiet test
> '
> diff --git a/ci/run-tests.sh b/ci/run-tests.sh
> index f0c743de94..c7aee5b9ff 100755
> --- a/ci/run-tests.sh
> +++ b/ci/run-tests.sh
> @@ -8,3 +8,7 @@
> mkdir -p $HOME/travis-cache
> ln -s $HOME/travis-cache/.prove t/.prove
> make --quiet test
> +if test "$jobname" = "linux-gcc"
> +then
> + GIT_TEST_SPLIT_INDEX=YesPlease make --quiet test
> +fi

For now I think that looks good. Maybe we could define additional test 
configurations with an environment variable. That could be an array variable
defined in the lib-travis.ci "case" statement:
https://github.com/git/git/blob/1229713f78cd2883798e95f33c19c81b523413fd/ci/lib-travisci.sh#L42-L65


- Lars

Re: [PATCH v1] convert: add support for 'encoding' attribute

[Junio C Hamano]

Torsten Bögershausen  writes:

> Just to confirm my missing knowledge here:
> Does this mean, that git-gui and gitk can decode/reencode
> the content of a file/blob, when the .gitattributes say so ?

These programs, when told that a file is in an encoding, read bytes
from that file and interpret that byte sequence in the given
encoding, and blits the corresponding glyphs on the screen---giving
UTF-8 out is not the primary feature of them, so I am not sure
decode/reencode is a good term to use here.

Re: [PATCH v2 1/3] repository: fix repo_read_index with submodules

[Brandon Williams]

On 12/17, Thomas Gummerer wrote:
> repo_read_index calls read_index_from, which takes an path argument for
> the location of the index file.  For the split index however it relies

> on the current working directory to construct the path using git_path.

This line isn't actually true and should probably be fixed.  git_path
doesn't rely on the CWD but rather it relies on the gitdir of the main
repository (the_repository).

> 
> repo_read_index calls read_index_from with the full path for the index
> file, however it doesn't change the cwd, so when split index mode is
> turned on, read_index_from can't find the file for the split index.
> 
> For example t3007-ls-files-recurse-submodules.sh was broken with
> GIT_TEST_SPLIT_INDEX set in 188dce131f ("ls-files: use repository
> object", 2017-06-22), and t7814-grep-recurse-submodules.sh was also
> broken in a similar manner, probably by introducing struct repository
> there, although I didn't track down the exact commit for that.
> 
> Fix this by introducing a new read_index_for_repo function, which knows
> about the correct paths for the submodules.
> 
> The alternative would have been to make the callers pass in the base
> path for the split index, however that ended up being more complicated,
> and I think we want to converge towards using struct repository for
> things like these anyway.
> 
> Signed-off-by: Thomas Gummerer 
> ---
>  cache.h  |  1 +
>  read-cache.c | 16 ++--
>  repository.c |  2 +-
>  3 files changed, 16 insertions(+), 3 deletions(-)
> 
> diff --git a/cache.h b/cache.h
> index cb5db7bf83..d42bea1ef7 100644
> --- a/cache.h
> +++ b/cache.h
> @@ -614,6 +614,7 @@ extern int read_index_preload(struct index_state *, const 
> struct pathspec *paths
>  extern int do_read_index(struct index_state *istate, const char *path,
>int must_exist); /* for testting only! */
>  extern int read_index_from(struct index_state *, const char *path);
> +extern int read_index_for_repo(const struct repository *);
>  extern int is_index_unborn(struct index_state *);
>  extern int read_index_unmerged(struct index_state *);
>  
> diff --git a/read-cache.c b/read-cache.c
> index 2eb81a66b9..70357febdc 100644
> --- a/read-cache.c
> +++ b/read-cache.c
> @@ -20,6 +20,7 @@
>  #include "split-index.h"
>  #include "utf8.h"
>  #include "fsmonitor.h"
> +#include "repository.h"
>  
>  /* Mask for the name length in ce_flags in the on-disk index */
>  
> @@ -1871,7 +1872,8 @@ static void freshen_shared_index(char *base_sha1_hex, 
> int warn)
>   free(shared_index);
>  }
>  
> -int read_index_from(struct index_state *istate, const char *path)
> +static int do_read_index_from(struct index_state *istate, const char *path,
> +   const struct repository *repo)
>  {
>   struct split_index *split_index;
>   int ret;
> @@ -1896,7 +1898,7 @@ int read_index_from(struct index_state *istate, const 
> char *path)
>   split_index->base = xcalloc(1, sizeof(*split_index->base));
>  
>   base_sha1_hex = sha1_to_hex(split_index->base_sha1);
> - base_path = git_path("sharedindex.%s", base_sha1_hex);
> + base_path = repo_git_path(repo, "sharedindex.%s", base_sha1_hex);
>   ret = do_read_index(split_index->base, base_path, 1);
>   if (hashcmp(split_index->base_sha1, split_index->base->sha1))
>   die("broken index, expect %s in %s, got %s",
> @@ -1909,6 +1911,16 @@ int read_index_from(struct index_state *istate, const 
> char *path)
>   return ret;
>  }
>  
> +int read_index_for_repo(const struct repository *repo)
> +{
> + return do_read_index_from(repo->index, repo->index_file, repo);
> +}

> +
> +int read_index_from(struct index_state *istate, const char *path)
> +{
> + return do_read_index_from(istate, path, the_repository);
> +}

This looks fine, though I wonder what the point of passing in the index
file even was since we end just ended up reading the 'sharedindex' file based
on the git path. I'm just curious about how this function evolved.

> +
>  int is_index_unborn(struct index_state *istate)
>  {
>   return (!istate->cache_nr && !istate->timestamp.sec);
> diff --git a/repository.c b/repository.c
> index bb2fae5446..928b1f553d 100644
> --- a/repository.c
> +++ b/repository.c
> @@ -229,5 +229,5 @@ int repo_read_index(struct repository *repo)
>   if (!repo->index)
>   repo->index = xcalloc(1, sizeof(*repo->index));
>  
> - return read_index_from(repo->index, repo->index_file);
> + return read_index_for_repo(repo);
>  }
> -- 
> 2.15.1.620.gb9897f4670
> 

-- 
Brandon Williams

Re: Q: rational for $XDG_CONFIG_HOME/git/config to be "non global" or just a bug?

[Junio C Hamano]

Yaroslav Halchenko  writes:

> On Mon, 18 Dec 2017, Jeff King wrote:
>
>> To complete that abstraction it seems like reading via "--global" should
>> read from both (in the same precedence order that normal config lookup
>> uses).
>
> FWIW +1 from me on that ;)

FWIW I do not have problem with that endgame.

I wonder if anybody wants to get their hands dirty.  It should be a
quite straight-forward to split a helper (or three) out of the
do_git_config_sequence() helper, but the interface from cmd_config()
to call into config.c to read a specific "class" of config file is
the same codepath as a single file, so a major part of this work
will be to design how to extend the interface to do the limited
"sequence" thing.

commit-msg hook possible bad behavior with -F

[Seth Raymond]

If a commit is invoked with -F , indicating that the commit
message should be read from an existing file, the the
prepare-commit-msg and commit-msg hooks do not operate on . The
first argument to the hook is always /COMMIT_EDITMSG, rather
than .

Am I wrong in this line of thinking?

-Seth

RE: [PATCH] p7519: improve check for prerequisite WATCHMAN

[Ben Peart]

> -Original Message-
> From: René Scharfe [mailto:l@web.de]
> Sent: Saturday, December 16, 2017 7:12 AM
> To: Git List 
> Cc: Junio C Hamano ; Ben Peart
> ; Ævar Arnfjörð Bjarmason
> 
> Subject: [PATCH] p7519: improve check for prerequisite WATCHMAN
> 
> The return code of command -v with a non-existing command is 1 in bash
> and 127 in dash.  Use that return code directly to allow the script to work
> with dash and without watchman (e.g. on Debian).
> 
> While at it stop redirecting the output.  stderr is redirected to /dev/null by
> test_lazy_prereq already, and stdout can actually be useful -- the path of the
> found watchman executable is sent there, but it's shown only if the script
> was run with --verbose.
> 
> Signed-off-by: Rene Scharfe 
> ---
>  t/perf/p7519-fsmonitor.sh | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/t/perf/p7519-fsmonitor.sh b/t/perf/p7519-fsmonitor.sh index
> 16d1bf72e5..65e145c02d 100755
> --- a/t/perf/p7519-fsmonitor.sh
> +++ b/t/perf/p7519-fsmonitor.sh
> @@ -40,8 +40,7 @@ test_lazy_prereq UNTRACKED_CACHE '
>  '
> 
>  test_lazy_prereq WATCHMAN '
> - { command -v watchman >/dev/null 2>&1; ret=$?; } &&
> - test $ret -ne 1
> + command -v watchman
>  '

Looks good to me.  I tested this on Windows and it still works with and without 
watchman.  Thanks for the update to get this working on other platforms.

> 
>  if test_have_prereq WATCHMAN
> --
> 2.15.1

Usability issue with rebase fork-point option

[Robert Dailey]

When upstream is not specified for the rebase command (e.g. I just do
`git rebase`), `--fork-point` is assumed which results in commits
regenerating SHA1 even if the merge-base would otherwise be identical.

Here's my scenario:

I set my remote tracking branch to my parent branch:

$ git branch -u origin

I do a status to see that I'm 1 ahead:

$ git status -sb
## feature/foo...origin/master [ahead 1]

I then execute `git rebase`:

$ git rebase
First, rewinding head to replay your work on top of it...
Applying: Fix state machine hang after integrity checking

Since my merge-base is already the tip of `origin/master`, I expected
it to say it was up-to-date, as it would if I disabled fork point:

$ git rebase --no-fork-point
Current branch feature/foo is up to date.


The expected behavior is that if merge-base does not change, even with
--fork-point enabled, that SHA1's do not get rewritten.

As a workaround, I've created an alias that always assumes --no-fork-point:

[alias]
rb = rebase --no-fork-point


Possible long term solutions I'd be happy with:

1. A config option to disable fork point completely, something like
`rebase.forkPoint` and I can set it to `false`.
2. Change rebase to always assume `--no-fork-point` as a default,
which is opposite of the current behavior.
3. Assuming the behavior I'm observing is a bug, put more priority on
a matching merge-base instead of the reflogs, not sure of the
internals of how this fork-point behavior is implemented, but this
feels like the issue to me. The most ideal outcome is that this is a
bug and no interface or config changes are needed.

Would love to get feedback from the Git community on this. Thanks for reading!!

Re: [PATCH] revision: introduce prepare_revision_walk_extended()

[Jeff King]

On Sat, Dec 16, 2017 at 01:12:16PM +0100, René Scharfe wrote:

> prepare_revision_walk() allows callers to take ownership of the array of
> pending objects by setting the rev_info flag "leak_pending" and copying
> the object_array "pending".  They use it to clear commit marks after
> setup is done.  This interface is brittle enough that it requires
> extensive comments.
> 
> Provide an easier way by adding a function that can hand over the array
> to a caller-supplied output parameter and converting all users of the
> flag "leak_pending" to call prepare_revision_walk_extended() instead.

I think this is _better_, but it's still kind of a funny interface.

The root of the matter is that the revision-walking code doesn't clean
up after itself. In every case, the caller is just saving these to clean
up commit marks, isn't it?

Could we instead have an interface like:

  revs.clear_commit_marks = 1;
  prepare_revision_walk();
  ...
  finish_revision_walk();

where that final function would do any cleanup, including clearing the
commit marks. I suspect there are other small bits that get leaked
because there's not really any "destructor" for a revision walk.

It's not as flexible as this whole "make a copy of the pending tips"
thing, but it keeps all of the details abstracted away from the callers.

Alternatively:

> +`prepare_revision_walk_extended`::
> +
> + Like prepare_revision_walk(), but allows callers to take ownership
> + of the array of pending objects by passing an object_array pointer
> + as the second parameter; passing NULL clears the array.

What if we just got rid of this function and had callers do:

  object_array_copy(_pending, );
  prepare_revision_walk();
  ...
  clear_commit_marks_for_object_array(_pending);

That sidesteps all of the memory ownership issues by just creating a
copy. That's less efficient, but I'd be surprised if it matters in
practice (we tend to do one or two revisions per process, there don't
tend to be a lot of pending tips, and we're really just talking about
copying some pointers here).

-Peff

[no subject]

[Bank Director]

[no subject]

[Director Bank]

Наш офисный контакт, 2554 Дороги Кпэлайм Фэйс Фармаки Бет, Ломе, Залив.

Международный валютный фонд (МВФ) дает компенсацию всем жертвам
жульничества, и Ваш адрес электронной почты был найден в list.this
Денежном грамме жертвы жульничества, и офис Western Union получил
мандат МВФ передать Вашу компенсацию Вам.


однако, мы завершили, чтобы затронуть Вашу собственную оплату
посредством денежного перевода Western Union, 5,000$ ежедневно, пока
полная сумма 850,000.00$ полностью не передана Вам. мы не можем быть в
состоянии послать оплату с одним только Вашим адресом электронной
почты, таким образом нам нужен Ваш
информация как, туда, где мы будем посылать денежные средства, такой как;

Ваш получать имя-
Страна получает-
Номер телефона
Копия паспорта

С уважением
Г-н Тони Вуд
ДИРЕКТОР ДЕНЕЖНОГО ГРАММА И WESTERN UNION ЛОМЕ-ТОГО.
Телефон, +22890272933

Re: Q: rational for $XDG_CONFIG_HOME/git/config to be "non global" or just a bug?

[Yaroslav Halchenko]

On Mon, 18 Dec 2017, Jeff King wrote:

> To complete that abstraction it seems like reading via "--global" should
> read from both (in the same precedence order that normal config lookup
> uses).

FWIW +1 from me on that ;)

-- 
Yaroslav O. Halchenko
Center for Open Neuroscience http://centerforopenneuroscience.org
Dartmouth College, 419 Moore Hall, Hinman Box 6207, Hanover, NH 03755
Phone: +1 (603) 646-9834   Fax: +1 (603) 646-1419
WWW:   http://www.linkedin.com/in/yarik

Re: [PATCH v1] convert: add support for 'encoding' attribute

[Jeff King]

On Mon, Dec 18, 2017 at 11:13:34AM +0100, Torsten Bögershausen wrote:

> Just to confirm my missing knowledge here:
> Does this mean, that git-gui and gitk can decode/reencode
> the content of a file/blob, when the .gitattributes say so ?

That's my impression, yes.

> If yes, would it make sense to enhance the "git diff" instead ?
> "git diff --encoding" will pick up the commited encoding from
> .attributes, convert it into UTF-8, and run the diff ?

You can do something like this already:

  git config diff.utf16.textconv 'iconv -f utf16 -t utf8'
  echo file diff=utf16 >.gitattributes

I have no objection to teaching it that "encoding" means roughly the
same thing, which would have two advantages:

 - we could do it in-process, which would be much faster

 - we could skip the extra config step, which is a blocker for
   server-side diffs on sites like GitHub

I don't think you'd really need "--encoding". This could just be
triggered by the normal "--textconv" rules (i.e., just treat this "as
if" the textconv above was configured when we see an encoding
attribute).

> We actually could enhance the "git diff" output with a single
> line saying
> "Git index-encoding=cp1251"
> or so, which can be picked up by "git apply".

That extends it beyond what textconv can do (we assume that textconv
patches are _just_ for human consumption, and can't be applied). And it
would mean you'd potentially want to trigger it in more places. On the
other hand, I don't know that we're guaranteed that a round-trip
between encodings will produce a byte-wise identical result. The nice
thing about piggy-backing on textconv is that it's already dealt with
that problem.

-Peff

Re: [PATCH v1] convert: add support for 'encoding' attribute

[Jeff King]

On Mon, Dec 18, 2017 at 11:54:32AM +0100, Lars Schneider wrote:

> >  warning: failed to encode 'file' from utf-8 to utf16
> > 
> > At least it figured out that it couldn't convert the content. It's
> > slightly troubling that it would try in the first place, though; are
> > there encoding pairs where we might accidentally generate nonsense?
> 
> At this point we interpret utf-16 content as utf-8 and try to convert
> it to utf-16. That of course fails because utf-16 content is no valid
> utf-8. How could we stop trying that? How could Git possibly know what
> kind of encoding is used (apart from our new hint in gitattributes)?

Yeah, sorry if I wasn't clear: I don't really have an answer to those
questions either. So this is probably the best we can do. I was mostly
just trying to think through the worst case, and what could go wrong.

> > It may make sense to die() during "git add ." (since we're actually
> > changing the index entry, and we don't want to put nonsense into a
> > tree). But I'm not sure it's the best thing for operations which just
> > want to read the content. For them, perhaps it would be more appropriate
> > to issue a warning and return the untouched content.
> 
> Absolutely! Thanks for spotting this. I will try to run die() only on
> "git add" in v2.

Great, thanks!

-Peff

Fetching commit instead of ref

[Carlsson, Magnus]

Hi

I am involved in the git-subrepo project 
(https://github.com/ingydotnet/git-subrepo/). It's an attempt to simplify the 
inclusion of repos into other repos.

In a certain situation I would really need to fetch all commits related to a 
specific commit (SHA). I have read the git fetch documentation and found 
nothing regarding this. It only seems to support fetching references.

I found some traces on stack overflow:
https://stackoverflow.com/questions/14872486/retrieve-specific-commit-from-a-remote-git-repository

Following that recommendation it feels like it almost works:
$ git fetch subrepo 
50f730db793e0733b159326c5a3e78fd48cedfec:refs/remote/subrepo/foo-commit
remote: Counting objects: 2311, done.
remote: Total 2311 (delta 0), reused 0 (delta 0), pack-reused 2311
Receiving objects: 100% (2311/2311), 703.64 KiB | 0 bytes/s, done.
Resolving deltas: 100% (1174/1174), done.
> So far so good, but then an error message appear:
error: Server does not allow request for unadvertised object 
50f730db793e0733b159326c5a3e78fd48cedfec
> And nothing seems to be fetched.

Is there a way to fetch a commit and any ancestors to that commit based on a 
SHA?

Why do I need this?
In git-subrepo we try to recreate another repo within our main repo. Creating 
the necessary parent references when they appear. In some cases we need to make 
sure that we have access to the correct commits from the subrepo, but we don't 
have any references except a SHA.
 
-- Magnus
 
 
MAGNUS CARLSSON
 Staff Software Engineer
 ARRIS
 
 o: +46 13 36 75 92
 e: magnus.carls...@arris.com
 w: www.arris.com
 
 ARRIS:  Legal entity: Arris Sweden AB - Registered Office: Teknikringen 2, 583 
30 Linkoping, Sweden - Reg No:556518-5831 - VAT No:SE 556518-583
 
 This electronic transmission (and any attached document) is for the sole use 
of the individual or entity to whom it is addressed.  It is confidential and 
may be attorney-client privileged.  In any event the Sender reserves, to the 
fullest extent, any "legal  advice privilege".  Any further distribution or 
copying of this message is strictly prohibited.  If you received this message 
in error, please notify the Sender immediately and destroy the attached message 
(and all attached documents).
  

Re: [PATCH v1] convert: add support for 'encoding' attribute

[Lars Schneider]

> On 15 Dec 2017, at 10:58, Jeff King  wrote:
> 
> On Mon, Dec 11, 2017 at 04:50:23PM +0100, lars.schnei...@autodesk.com wrote:
> 
>> From: Lars Schneider 
>> 
>> Git and its tools (e.g. git diff) expect all text files in UTF-8
>> encoding. Git will happily accept content in all other encodings, too,
>> but it might not be able to process the text (e.g. viewing diffs or
>> changing line endings).
>> 
>> Add an attribute to tell Git what encoding the user has defined for a
>> given file. If the content is added to the index, then Git converts the
>> content to a canonical UTF-8 representation. On checkout Git will
>> reverse the conversion.
> 
> This made me wonder what happens when you have a file that _was_ utf16,
> and then you later convert it to utf8 and add a .gitattributes entry.
> 
> I tried this with your patch:
> 
>  git init repo
>  cd repo
> 
>  echo foo | iconv -t utf16 >file
>  git add file
>  git commit -m utf16
> 
>  echo 'file encoding=utf16' >.gitattributes
>  touch file ;# make it stat-dirty
>  git commit -m convert
> 
>  git checkout HEAD^
> 
> That works OK, because we try to read the attributes from the
> destination tree for a checkout. If you do this:
> 
>  echo 'file encoding=utf16' >.git/info/attributes
>  git checkout HEAD^
> 
> then we get:
> 
>  warning: failed to encode 'file' from utf-8 to utf16
> 
> At least it figured out that it couldn't convert the content. It's
> slightly troubling that it would try in the first place, though; are
> there encoding pairs where we might accidentally generate nonsense?

At this point we interpret utf-16 content as utf-8 and try to convert
it to utf-16. That of course fails because utf-16 content is no valid
utf-8. How could we stop trying that? How could Git possibly know what
kind of encoding is used (apart from our new hint in gitattributes)?

I asked myself the question about nonsense encoding pairs, too. That
was the reason I added the "X -> UTF-8 -> Y && X == Y" check on Git
add. However, with ".git/info/attributes" you could circumvent that
check and probably generate nonsense.


> It _should_ be uncommon, I think, to have a repo-level attribute set
> like that. It's very common for us to use whatever happens to be in the
> checked-out .gitattributes for some attributes (e.g., when doing a diff
> of an older commit), but I think for checkout-related ones it's not.  So
> I think it may generally work in practice. And certainly the line-ending
> code would share any similar problems, but at least there the result is
> less confusing than mojibake.
> 
> Playing around, I also managed to do this:
> 
>  echo 'file encoding=utf16' >.gitattributes
>  echo foo >file
> 
>  # I did these with an old version of git that didn't
>  # support the new attribute, so they blindly added the utf8 content.
>  git add .
>  git commit -m convert
> 
>  git.compile checkout HEAD^
> 
> which yielded:
> 
>  fatal: encoding 'file' from utf16 to utf-8 and back is not the same
> 
> Now obviously my situation is somewhat nonsense. I was trying to force
> the in-repo representation to utf8, but ended up with a mismatched
> working tree file. But what's somewhat troubling is that I couldn't
> checkout _away_ from that state due to the die() in convert_to_git().
> Which is in turn just there as part of refresh_index().
> 
> And indeed, other commands hit the same problem:
> 
>  $ git.compile diff
>  fatal: encoding 'file' from utf16 to utf-8 and back is not the same
> 
>  $ git.compile checkout -f
>  fatal: encoding 'file' from utf16 to utf-8 and back is not the same
> 
> It may make sense to die() during "git add ." (since we're actually
> changing the index entry, and we don't want to put nonsense into a
> tree). But I'm not sure it's the best thing for operations which just
> want to read the content. For them, perhaps it would be more appropriate
> to issue a warning and return the untouched content.

Absolutely! Thanks for spotting this. I will try to run die() only on
"git add" in v2.

- Lars

Re: [PATCH v1] convert: add support for 'encoding' attribute

[Torsten Bögershausen]

On Mon, Dec 11, 2017 at 09:47:24PM +0100, Johannes Sixt wrote:
> Am 11.12.2017 um 16:50 schrieb lars.schnei...@autodesk.com:
> >From: Lars Schneider 
> >
> >Git and its tools (e.g. git diff) expect all text files in UTF-8
> >encoding. Git will happily accept content in all other encodings, too,
> >but it might not be able to process the text (e.g. viewing diffs or
> >changing line endings).
> >
> >Add an attribute to tell Git what encoding the user has defined for a
> >given file. If the content is added to the index, then Git converts the
> >content to a canonical UTF-8 representation. On checkout Git will
> >reverse the conversion.
> >
> >Reviewed-by: Patrick Lühne 
> >Signed-off-by: Lars Schneider 
> >---
> >
> >Hi,
> >
> >here is a WIP patch to add text encoding support for files encoded with
> >something other than UTF-8 [RFC].
> >
> >The 'encoding' attribute is already used to view blobs in gitk. That
> >could be a problem as the content is stored in Git with the defined
> >encoding. This patch would interpret the content as UTF-8 encoded and
> 
> This will be a major drawback for me because my code base stores text files
> that are not UTF-8 encoded. And I do use the existing 'encoding' attribute
> to view the text in git-gui and gitk. Repurposing this attribute name is not
> an option, IMO.

Just to confirm my missing knowledge here:
Does this mean, that git-gui and gitk can decode/reencode
the content of a file/blob, when the .gitattributes say so ?

If yes, would it make sense to enhance the "git diff" instead ?
"git diff --encoding" will pick up the commited encoding from
.attributes, convert it into UTF-8, and run the diff ?
We actually could enhance the "git diff" output with a single
line saying
"Git index-encoding=cp1251"
or so, which can be picked up by "git apply".

The advantage would be that we could continue to commit in UTF-16
as before, and avoid the glitches with .gitattributes, that Peff
pointed out.

Does this make sense ?

> 
> >it would try to reencode it to the defined encoding on checkout > Plus,
> >many repos define the attribute very broad (e.g. "*
> encoding=cp1251").

Is this a user mistake ?

> >These folks would see errors like these with my patch:
> > error: failed to encode 'foo.bar' from utf-8 to cp1251
> 
> -- Hannes

Draft of Git Rev News edition 34

[Christian Couder]

Hi,

A draft of a new Git Rev News edition is available here:

  https://github.com/git/git.github.io/blob/master/rev_news/drafts/edition-34.md

Everyone is welcome to contribute in any section either by editing the
above page on GitHub and sending a pull request, or by commenting on
this GitHub issue:

  https://github.com/git/git.github.io/issues/267

You can also reply to this email.

In general all kinds of contribution, for example proofreading,
suggestions for articles or links, help on the issues in GitHub, and
so on, are very much appreciated. This is even more relevant than
usual  as Thomas is stepping down from editing the "Releases" and
"Other News" sections (see the draft for more information).

I tried to cc everyone who appears in this edition, but maybe I missed
some people, sorry about that.

Jakub, Markus and myself plan to publish this edition on
Wednesday December 20th.

Thanks,
Christian.

Hallo

[Meinze Klaus Peter]

 Greeting

In a brief introduction, I am a lawyer Meinze klaus peter,from Germany
but now i lives in London, I sent you  an email about your deceased
relative, but I have not received any response from you, deceased is a
citizen of  your  country with the same surname with you, he is an
exporter of gold here in London,

'He died a few years ago with his family, leaving his company,and huge
amounts of money deposited in UBS Investment Bank here London,

I'm his personal lawyer to the late clinet and I need your cooperation
So that we can get the money from the bank before the government
finally seize it, the Total amount in the bank is  £ 5.7 million, GBP
but will explain,more details if I hear from you


God be with you???