Re: [PATCH] Fix http-backend reading till EOF, ignoring CONTENT_LENGTH, violating rfc3875 -- WAS: Problem with git-http-backend.exe as iis cgi

[Jeff King]

On Wed, Mar 30, 2016 at 09:08:56AM +, Florian Manschwetus wrote:

> After additional analysis it turned out, that in the case you
> mentioned, at least IIS, sets CONTENT_LENGTH to -1 resulting in the
> current behavior of git-http-backend being sufficient in this
> situation.
> Therefore I refactored the code again a bit, to match up the behavior
> I currently fake by using some bash magic...

OK, so I'd agree it makes sense to catch "-1", and read to EOF in that
case (or if CONTENT_LENGTH is NULL).

> From ccd6c88e39a850b253979b785463719cdc0fa1e2 Mon Sep 17 00:00:00 2001
> From: manschwetus 
> Date: Tue, 29 Mar 2016 12:16:21 +0200
> Subject: [PATCH 1/2] Fix http-backend reading till EOF, ignoring
>  CONTENT_LENGTH, violating rfc3875

Please send one patch per email, and these header bits should be the
header of your email.

Though we also generally revise and re-send patches, rather than
presenting one patch that has problems and then tacking fixes on top.
I'll ignore the problems in this patch 1, as it looks like it's just the
original one repeated.

> From 4b2aac3dfd4954098190745a9e4fa17f254cd6a1 Mon Sep 17 00:00:00 2001
> From: Florian Manschwetus 
> Date: Wed, 30 Mar 2016 10:54:21 +0200
> Subject: [PATCH 2/2] restored old behavior as read_request_eof(...) and moved
>  new variant to read_request_fix_len(...) and introduced read_request(...) as
>  wrapper, which decides based on value retrieved from CONTENT_LENGTH which
>  variant to use

Please use a short subject for your commit message, followed by a blank
line and then a more explanatory body. Also, don't just describe _what_
is happening (we can see that from the diff), but _why_. You can find
more similar tips in SubmittingPatches.

> +/**
> + * wrapper function, whcih determines based on CONTENT_LENGTH value,
> + * to
> + * - use old behaviour of read_request, to read until EOF
> + * => read_request_eof(...)
> + * - just read CONTENT_LENGTH-bytes, when provided
> + * => read_request_fix_len(...)
> + */
> +static ssize_t read_request(int fd, unsigned char **out)
> +{
> +/* get request size */
> +size_t req_len = git_env_ulong("CONTENT_LENGTH",
> +   -1);
> +if (req_len < 0){
> +  read_request_eof(fd, out);
> +} else {
> +  read_request_fix_len(fd, req_len, out);
> +}
> +}

I don't think "if (req_len < 0)" can ever trigger, because size_t is an
unsigned type (and I do not recall what kind of integer overflow
validation we do in git_env_ulong, but I suspect it may complain about
"-1"). You may have to parse the variable manually rather than using
git_env_ulong (i.e., pick out the NULL and "-1" cases, and then feed the
rest to git_parse_ulong()).

Also, a few style nits. We usually omit braces for one-line
conditionals, and please make sure there is whitespace between the
closing parenthesis and the opening brace. There's some discussing in
CodingGuidelines.

-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

What's cooking in git.git (Apr 2016, #01; Fri, 1)

[Junio C Hamano]

Here are the topics that have been cooking.  Commits prefixed with
'-' are only in 'pu' (proposed updates) while commits prefixed with
'+' are in 'next'.  The ones marked with '.' do not appear in any of
the integration branches, but I am still holding onto them.

There are quite a few topics that have been cooking in 'next' during
the pre-2.8 freeze period that are ready to be merged to 'master';
the first batch will be merged early next week, and then the tip of
'next' will be rebuilt on top sometime mid next week.  Let's start
looking at new shiny toys after all that happens.

You can find the changes described here in the integration branches
of the repositories listed at

http://git-blame.blogspot.com/p/git-public-repositories.html

--
[New Topics]

* jn/mergetools-examdiff (2016-03-28) 2 commits
 - mergetools: add support for ExamDiff
 - mergetools: create mergetool_find_win32_cmd() helper function for winmerge

 "git mergetools" learned to drive ExamDiff.

 Waiting for an Ack by the area expert.


* kn/for-each-tag-branch (2016-03-30) 1 commit
 - for-each-ref: fix description of '--contains' in manpage

 Docfix.

 Will merge to 'next'.


* kn/ref-filter-branch-list (2016-03-30) 16 commits
 . branch: implement '--format' option
 . branch: use ref-filter printing APIs
 . branch, tag: use porcelain output
 . ref-filter: allow porcelain to translate messages in the output
 . ref-filter: add support for %(refname:dir) and %(refname:base)
 . ref-filter: introduce refname_atom_parser()
 . ref-filter: introduce symref_atom_parser()
 . ref-filter: make "%(symref)" atom work with the ':short' modifier
 . ref-filter: add support for %(upstream:track,nobracket)
 . ref-filter: make %(upstream:track) prints "[gone]" for invalid upstreams
 . ref-filter: introduce format_ref_array_item()
 . ref-filter: move get_head_description() from branch.c
 . ref-filter: modify "%(objectname:short)" to take length
 . ref-filter: implement %(if:equals=) and %(if:notequals=)
 . ref-filter: include reference to 'used_atom' within 'atom_value'
 . ref-filter: implement %(if), %(then), and %(else) atoms

 The code to list branches in "git branch" has been consolidated
 with the more generic ref-filter API.

 Will be rerolled.


* oa/doc-diff-check (2016-03-29) 1 commit
 - Documentation: git diff --check detects conflict markers

 Docfix.

 Will merge to 'next'.


* rz/worktree-no-checkout (2016-03-29) 1 commit
 - worktree: add: introduce --checkout option

 "git worktree add" can be given "--no-checkout" option to only
 create an empty worktree without checking out the files.

 Will merge to 'next'.


* sb/misc-cleanups (2016-04-01) 4 commits
 - credential-cache, send_request: close fd when done
 - bundle: don't leak an fd in case of early return
 - abbrev_sha1_in_line: don't leak memory
 - notes: don't leak memory in git_config_get_notes_strategy

 Assorted minor clean-ups.

 Will merge to 'next'.


* sb/submodule-helper-clone-regression-fix (2016-04-01) 6 commits
 - submodule--helper, module_clone: catch fprintf failure
 - submodule--helper: do not borrow absolute_path() result for too long
 - submodule--helper, module_clone: always operate on absolute paths
 - submodule--helper clone: create the submodule path just once
 - submodule--helper: fix potential NULL-dereference
 - recursive submodules: test for relative paths

 A partial rewrite of "git submodule" in the 2.7 timeframe changed
 the way the gitdir: pointer in the submodules point at the real
 repository location to use absolute paths by accident.  This has
 been corrected.

 Any further comments?  Otherwise will merge to 'next'.


* sb/submodule-path-misc-bugs (2016-03-30) 6 commits
 - t7407: make expectation as clear as possible
 - submodule update: test recursive path reporting from subdirectory
 - submodule update: align reporting path for custom command execution
 - submodule status: correct path handling in recursive submodules
 - submodule update --init: correct path handling in recursive submodules
 - submodule foreach: correct path display in recursive submodules

 "git submodule" reports the paths of submodules the command
 recurses into, but this was incorrect when the command was not run
 from the root level of the superproject.

 Any further comments?  Otherwise will merge to 'next'.


* sg/diff-multiple-identical-renames (2016-03-30) 1 commit
 - diffcore: fix iteration order of identical files during rename detection

 "git diff -M" used to work better when two originally identical
 files A and B got renamed to X/A and X/B by pairing A to X/A and B
 to X/B, but this was broken in 2.0 timeframe.

 Will merge to 'next'.


* sk/send-pack-all-fix (2016-03-31) 1 commit
 - git-send-pack: fix --all option when used with directory

 "git send-pack --all " was broken when its command line
 option parsing was written in 2.6 timeframe.

 Will merge to 'next'.


* ss/msvc (2016-03-30) 2 commits
 - MSVC: use shipped 

Re: [PATCH v2 6/7] correct blame for files commited with CRLF

[Junio C Hamano]

tbo...@web.de writes:

> Whenever a CLRF conversion is needed (or any filter us set), load the
> index temporally, before calling convert_to_git()

I am not sure if this is needed.  We should just do read_cache()
unconditionally at the beginning of fake_working_tree_commit(),
without even discarding it.  Most Git commands that work on the
working tree files follow that pattern, and I do not think the
"optimization" this patch does over that simple pattern, while it
may be nice, is an unnecessary complication.

By the way, thanks for pushing back in the [v1 6/7] discussion
thread to show me where convert_to_git() was failing.

> Signed-off-by: Torsten Bögershausen 
> ---
>  builtin/blame.c   |  6 +-
>  convert.c | 10 ++
>  convert.h |  1 +
>  t/t8003-blame-corner-cases.sh | 14 ++
>  4 files changed, 30 insertions(+), 1 deletion(-)
>
> diff --git a/builtin/blame.c b/builtin/blame.c
> index e982fb8..a219068 100644
> --- a/builtin/blame.c
> +++ b/builtin/blame.c
> @@ -2376,7 +2376,11 @@ static struct commit *fake_working_tree_commit(struct 
> diff_options *opt,
>   if (strbuf_read(, 0, 0) < 0)
>   die_errno("failed to read from stdin");
>   }
> - convert_to_git(path, buf.buf, buf.len, , 0);
> + if (convert_needs_conversion(path)) {
> + read_cache();
> + convert_to_git(path, buf.buf, buf.len, , 0);
> + discard_cache();
> + }
>   origin->file.ptr = buf.buf;
>   origin->file.size = buf.len;
>   pretend_sha1_file(buf.buf, buf.len, OBJ_BLOB, origin->blob_sha1);
> diff --git a/convert.c b/convert.c
> index 4ed5d89..02c50da 100644
> --- a/convert.c
> +++ b/convert.c
> @@ -903,6 +903,16 @@ const char *get_convert_attr_ascii(const char *path)
>   return "";
>  }
>  
> +int convert_needs_conversion(const char *path)
> +{
> + struct conv_attrs ca;
> +
> + convert_attrs(, path);
> + if (ca.drv || ca.ident || ca.crlf_action != CRLF_BINARY)
> + return 1;
> + return 0;
> +}
> +
>  int convert_to_git(const char *path, const char *src, size_t len,
> struct strbuf *dst, enum safe_crlf checksafe)
>  {
> diff --git a/convert.h b/convert.h
> index ccf436b..ffd9c32 100644
> --- a/convert.h
> +++ b/convert.h
> @@ -35,6 +35,7 @@ extern enum eol core_eol;
>  extern const char *get_cached_convert_stats_ascii(const char *path);
>  extern const char *get_wt_convert_stats_ascii(const char *path);
>  extern const char *get_convert_attr_ascii(const char *path);
> +int convert_needs_conversion(const char *path);
>  
>  /* returns 1 if *dst was used */
>  extern int convert_to_git(const char *path, const char *src, size_t len,
> diff --git a/t/t8003-blame-corner-cases.sh b/t/t8003-blame-corner-cases.sh
> index 6568429..a9b266f 100755
> --- a/t/t8003-blame-corner-cases.sh
> +++ b/t/t8003-blame-corner-cases.sh
> @@ -212,4 +212,18 @@ test_expect_success 'blame file with CRLF attributes 
> text' '
>   grep "A U Thor" actual
>  '
>  
> +test_expect_success 'blame file with CRLF core.autocrlf=true' '
> + git config core.autocrlf false &&
> + printf "testcase\r\n" >crlfinrepo &&
> + >.gitattributes &&
> + git add crlfinrepo &&
> + git commit -m "add crlfinrepo" &&
> + git config core.autocrlf true &&
> + mv crlfinrepo tmp &&
> + git checkout crlfinrepo &&
> + rm tmp &&
> + git blame crlfinrepo >actual &&
> + grep "A U Thor" actual
> +'
> +
>  test_done
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 5/7] CRLF: unify the "auto" handling

[Junio C Hamano]

tbo...@web.de writes:

> From: Torsten Bögershausen 
>
> Make .gitattributes "* text=auto eol=crlf" to do the same as
> setting core.autocrlf=true and "* text=auto eol=crlf" the same
> as core.autocrlf=input

"Earlier we didn't do so and instead did X, which was not great
because it caused symptom Y and Z"

would be necessary for future readers to understand why the above
change is a good idea.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 4/7] t0027: TC for combined attributes

[Junio C Hamano]

tbo...@web.de writes:

> +for crlf in true false input;

Style: lose the ';' at the end (there may be other instances of the
same).

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 3/7] Allow core.autocrlf=input and core.eol=crlf

[Junio C Hamano]

tbo...@web.de writes:

> From: Torsten Bögershausen 
>
> Commit  942e77 "Add "core.eol" config variable" adds a condition to
> the config parser: core.autocrlf=input is not allowed together with
> core.eol=crlf.
>
> This may lead to unnecessary problems, when config files are parsed:
> A global config file sets core.autocrlf=input,
> the repo local config file sets is own configuration: core.eol=crlf

This paragraph is unparsable.  Do you mean

When the global configuration file sets core.autocrlf=input
and the repository local one sets core.eol=crlf, they violate
this condition and triggers an error when the configuration
is read.

I think that is the designed behaviour, so it is not "a problem" at
all, and an example that makes them come from two different sources
of configuration is a red herring.  The parser faithfully implement
the policy that the combination is invalid.

The "problem" is in the policy itself, which you address below.

> There is no need to prevent combinations in config.c, in any case
> core.autocrlf overrides core.eol.

So

Even though the configuration parser errors out when
core.autocrlf is set to 'input' when core.eol is set to
'crlf', there is no need to do so, because the core.autocrlf
setting trumps core.eol.

or something like that, perhaps?

> Allow all combinations of core.crlf and core.eol and document
> that core.autocrlf overrides core.eol.

That is a good change.

> Signed-off-by: Torsten Bögershausen 
> ---
>  Documentation/config.txt | 6 +++---
>  config.c | 4 
>  2 files changed, 3 insertions(+), 7 deletions(-)
>
> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index 2cd6bdd..4a27ad4 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -337,9 +337,9 @@ core.quotePath::
>  
>  core.eol::
>   Sets the line ending type to use in the working directory for
> - files that have the `text` property set.  Alternatives are
> - 'lf', 'crlf' and 'native', which uses the platform's native
> - line ending.  The default value is `native`.  See
> + files that have the `text` property set when core.autocrlf is false.
> + Alternatives are 'lf', 'crlf' and 'native', which uses the platform's
> + native line ending.  The default value is `native`.  See
>   linkgit:gitattributes[5] for more information on end-of-line
>   conversion.
>  
> diff --git a/config.c b/config.c
> index 9ba40bc..a6adc8b 100644
> --- a/config.c
> +++ b/config.c
> @@ -803,8 +803,6 @@ static int git_default_core_config(const char *var, const 
> char *value)
>  
>   if (!strcmp(var, "core.autocrlf")) {
>   if (value && !strcasecmp(value, "input")) {
> - if (core_eol == EOL_CRLF)
> - return error("core.autocrlf=input conflicts 
> with core.eol=crlf");
>   auto_crlf = AUTO_CRLF_INPUT;
>   return 0;
>   }
> @@ -830,8 +828,6 @@ static int git_default_core_config(const char *var, const 
> char *value)
>   core_eol = EOL_NATIVE;
>   else
>   core_eol = EOL_UNSET;
> - if (core_eol == EOL_CRLF && auto_crlf == AUTO_CRLF_INPUT)
> - return error("core.autocrlf=input conflicts with 
> core.eol=crlf");
>   return 0;
>   }
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] ident: make the useConfigOnly error messages more informative

[Junio C Hamano]

Marios Titas  writes:

> Yeah, maybe informative is not the right word. What I meant is that it
> directs the user to do the "git config user.name" thing, which is
> likely the most appropriate course of action in this situation. In any
> event, I think printing the env_hint message would be really helpful
> in this case.

OK, let's do this, then.

Thanks.

-- >8 --
From: Marios Titas 
Date: Wed, 30 Mar 2016 22:29:43 +0300
Subject: [PATCH] ident: give "please tell me" message upon useConfigOnly error

The env_hint message applies perfectly to the case when
user.useConfigOnly is set and at least one of the user.name and the
user.email are not provided.

Additionally, use a less descriptive error message to discourage
users from disabling user.useConfigOnly configuration variable to
work around this error condition.  We want to encourage them to set
user.name or user.email instead.

Signed-off-by: Marios Titas 
Signed-off-by: Junio C Hamano 
---
 ident.c | 12 
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/ident.c b/ident.c
index b2521ff..c766127 100644
--- a/ident.c
+++ b/ident.c
@@ -352,8 +352,10 @@ const char *fmt_ident(const char *name, const char *email,
int using_default = 0;
if (!name) {
if (strict && ident_use_config_only
-   && !(ident_config_given & IDENT_NAME_GIVEN))
-   die("user.useConfigOnly set but no name given");
+   && !(ident_config_given & IDENT_NAME_GIVEN)) {
+   fputs(env_hint, stderr);
+   die("no name was given and auto-detection is 
disabled");
+   }
name = ident_default_name();
using_default = 1;
if (strict && default_name_is_bogus) {
@@ -375,8 +377,10 @@ const char *fmt_ident(const char *name, const char *email,
 
if (!email) {
if (strict && ident_use_config_only
-   && !(ident_config_given & IDENT_MAIL_GIVEN))
-   die("user.useConfigOnly set but no mail given");
+   && !(ident_config_given & IDENT_MAIL_GIVEN)) {
+   fputs(env_hint, stderr);
+   die("no email was given and auto-detection is 
disabled");
+   }
email = ident_default_email();
if (strict && default_email_is_bogus) {
fputs(env_hint, stderr);
-- 
2.8.0-246-g1783343

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] ident: check for useConfigOnly before auto-detection of name/email

[Junio C Hamano]

Marios Titas  writes:

> On Thu, Mar 31, 2016 at 10:40:03AM -0400, Jeff King wrote:
>>On Wed, Mar 30, 2016 at 10:29:42PM +0300, Marios Titas wrote:
>>
>>> If user.useConfigOnly is set, it does not make sense to try to
>>> auto-detect the name and/or the email. So it's better to do the
>>> useConfigOnly checks first.
>>
>>It might be nice to explain how it is better here. I'd guess it is
>>because we may fail during xgetpwuid(), giving a message that is much
>>less informative?
>
> Oops sorry, my bad, I should have included an example in the commit
> message. So with git 2.8.0, if you provide a name and set
> useConfigOnly to true in your ~/.gitconfig file, then if try to commit
> something in a new git repo, it will fail with the following message:
>
>*** Please tell me who you are.
> Run
>   git config --global user.email "y...@example.com"
>  git config --global user.name "Your Name"
> to set your account's default identity.
>Omit --global to set the identity only in this repository.
> fatal: unable to auto-detect email address (got 'XXX@YYY.(none)')
>
> (provided of course that auto-detection of email fails). This wrong,
> because auto-detection is disabled anyway.

OK, let's do this, then.

-- >8 --
From: Marios Titas 
Date: Wed, 30 Mar 2016 22:29:42 +0300
Subject: [PATCH] ident: check for useConfigOnly before auto-detection of
 name/email

If user.useConfigOnly is set, it does not make sense to try to
auto-detect the name and/or the email.  The auto-detection may
even result in a bogus name and trigger an error message.

Check if the use-config-only is set and die if no explicit name was
given, before attempting to auto-detect, to correct this.

Signed-off-by: Marios Titas 
Signed-off-by: Junio C Hamano 
---
 ident.c | 12 ++--
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/ident.c b/ident.c
index 4bd8084..b2521ff 100644
--- a/ident.c
+++ b/ident.c
@@ -351,15 +351,15 @@ const char *fmt_ident(const char *name, const char *email,
if (want_name) {
int using_default = 0;
if (!name) {
+   if (strict && ident_use_config_only
+   && !(ident_config_given & IDENT_NAME_GIVEN))
+   die("user.useConfigOnly set but no name given");
name = ident_default_name();
using_default = 1;
if (strict && default_name_is_bogus) {
fputs(env_hint, stderr);
die("unable to auto-detect name (got '%s')", 
name);
}
-   if (strict && ident_use_config_only
-   && !(ident_config_given & IDENT_NAME_GIVEN))
-   die("user.useConfigOnly set but no name given");
}
if (!*name) {
struct passwd *pw;
@@ -374,14 +374,14 @@ const char *fmt_ident(const char *name, const char *email,
}
 
if (!email) {
+   if (strict && ident_use_config_only
+   && !(ident_config_given & IDENT_MAIL_GIVEN))
+   die("user.useConfigOnly set but no mail given");
email = ident_default_email();
if (strict && default_email_is_bogus) {
fputs(env_hint, stderr);
die("unable to auto-detect email address (got '%s')", 
email);
}
-   if (strict && ident_use_config_only
-   && !(ident_config_given & IDENT_MAIL_GIVEN))
-   die("user.useConfigOnly set but no mail given");
}
 
strbuf_reset();
-- 
2.8.0-246-g1783343

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] submodule--helper, module_clone: always operate on absolute paths

[Junio C Hamano]

Junio C Hamano  writes:

> From: Junio C Hamano 
> Date: Fri, 1 Apr 2016 12:23:16 -0700
> Subject: [PATCH] submodule--helper: do not borrow absolute_path() result for 
> too long
>
> absolute_path() is designed to allow its callers to take a brief
> peek of the result (typically, to be fed to functions like
> strbuf_add() and relative_path() as a parameter) without having to
> ...
> @@ -199,8 +198,7 @@ static int module_clone(int argc, const char **argv, 
> const char *prefix)
>   die(_("submodule--helper: unspecified or empty --path"));
>  
>   strbuf_addf(, "%s/modules/%s", get_git_dir(), name);
> - sm_gitdir_rel = strbuf_detach(, NULL);
> - sm_gitdir = absolute_path(sm_gitdir_rel);
> + sm_gitdir = xstrdup(absolute_path(sb.buf));

Just to prevent others from wasting time scratching their heads,
we need

strbuf_reset();

here, as the strbuf will be reused later and is expected to be empty
at this point.

>  
>   if (!is_absolute_path(path)) {
>   strbuf_addf(, "%s/%s", get_git_work_tree(), path);
> @@ -245,7 +243,7 @@ static int module_clone(int argc, const char **argv, 
> const char *prefix)
>  relative_path(path, sm_gitdir, _path));
>   strbuf_release();
>   strbuf_release(_path);
> - free(sm_gitdir_rel);
> + free(sm_gitdir);
>   free(path);
>   free(p);
>   return 0;
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] imap-send.c: implements the GIT_CURL_DEBUG environment variable

[Eric Sunshine]

On Fri, Apr 1, 2016 at 6:44 AM, Elia Pinto  wrote:
> Implements the GIT_CURL_DEBUG environment variable to allow a greater
> degree of detail of GIT_CURL_VERBOSE, in particular the complete
> transport header and all the data payload exchanged.
> It might be useful if a particular situation could require a more
> thorough debugging analysis.

In addition to review comments by others, why are the new curl_dump()
and curl_trace() functions duplicated in both patches rather than
factored out to a shared implementation?

> Signed-off-by: Elia Pinto 
> ---
> diff --git a/imap-send.c b/imap-send.c
> @@ -1395,6 +1395,96 @@ static int append_msgs_to_imap(struct imap_server_conf 
> *server,
>  }
>
>  #ifdef USE_CURL_FOR_IMAP_SEND
> +
> +static
> +void curl_dump(const char *text,
> + FILE * stream, unsigned char *ptr, size_t size, char nohex)
> +{
> +   size_t i;
> +   size_t c;
> +
> +   unsigned int width = 0x10;
> +
> +   if (nohex)
> +   /* without the hex output, we can fit more on screen */
> +   width = 0x40;
> +
> +   fprintf(stream, "%s, %10.10ld bytes (0x%8.8lx)\n",
> +   text, (long)size, (long)size);
> +
> +   for (i = 0; i < size; i += width) {
> +
> +   fprintf(stream, "%4.4lx: ", (long)i);
> +
> +   if (!nohex) {
> +   /* hex not disabled, show it */
> +   for (c = 0; c < width; c++)
> +   if (i + c < size)
> +   fprintf(stream, "%02x ", ptr[i + c]);
> +   else
> +   fputs("   ", stream);
> +   }
> +
> +   for (c = 0; (c < width) && (i + c < size); c++) {
> +   /* check for 0D0A; if found, skip past and start a 
> new line of output */
> +   if (nohex && (i + c + 1 < size) && ptr[i + c] == 0x0D
> +   && ptr[i + c + 1] == 0x0A) {
> +   i += (c + 2 - width);
> +   break;
> +   }
> +   fprintf(stream, "%c",
> +   (ptr[i + c] >= 0x20)
> +   && (ptr[i + c] < 0x80) ? ptr[i + c] : '.');
> +   /* check again for 0D0A, to avoid an extra \n if it's 
> at width */
> +   if (nohex && (i + c + 2 < size)
> +   && ptr[i + c + 1] == 0x0D
> +   && ptr[i + c + 2] == 0x0A) {
> +   i += (c + 3 - width);
> +   break;
> +   }
> +   }
> +   fputc('\n', stream);/* newline */
> +   }
> +   fflush(stream);
> +}
> +
> +static
> +int curl_trace(CURL * handle, curl_infotype type,
> +char *data, size_t size, void *userp)
> +{
> +   const char *text;
> +   (void)handle;   /* prevent compiler warning */
> +
> +   switch (type) {
> +   case CURLINFO_TEXT:
> +   fprintf(stderr, "== Info: %s", data);
> +   default:/* in case a new one is introduced to shock 
> us */
> +   return 0;
> +
> +   case CURLINFO_HEADER_OUT:
> +   text = "=> Send header";
> +   break;
> +   case CURLINFO_DATA_OUT:
> +   text = "=> Send data";
> +   break;
> +   case CURLINFO_SSL_DATA_OUT:
> +   text = "=> Send SSL data";
> +   break;
> +   case CURLINFO_HEADER_IN:
> +   text = "<= Recv header";
> +   break;
> +   case CURLINFO_DATA_IN:
> +   text = "<= Recv data";
> +   break;
> +   case CURLINFO_SSL_DATA_IN:
> +   text = "<= Recv SSL data";
> +   break;
> +   }
> +
> +   curl_dump(text, stderr, (unsigned char *)data, size, 1);
> +   return 0;
> +}
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] submodule--helper, module_clone: always operate on absolute paths

[Eric Sunshine]

On Fri, Apr 1, 2016 at 3:30 PM, Junio C Hamano  wrote:
> Subject: [PATCH] submodule--helper: do not borrow absolute_path() result for 
> too long
>
> absolute_path() is designed to allow its callers to take a brief
> peek of the result (typically, to be fed to functions like
> strbuf_add() and relative_path() as a parameter) without having to
> worry about freeing it, but the other side of the coin of that
> memory model is that the caller shouldn't rely too much on the
> result living forever--there may be a helper function the caller
> subsequently calls that makes its own call to absolute_path(),
> invalidating the earlier result.
>
> Use xstrdup() to make our own copy, and free(3) it when we are done.
> While at it, remove an unnecessary sm_gitdir_rel variable that was
> only used to as a parameter to call absolute_paht() and never used

s/absolute_paht/absolute_path/

> again.
>
> Signed-off-by: Junio C Hamano 
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 00/16] port branch.c to use ref-filter's printing options

[Karthik Nayak]

On Fri, Apr 1, 2016 at 8:45 PM, Ramsay Jones
 wrote:
>
>
> On 01/04/16 14:44, Dennis Kaarsemaker wrote:
>> On Fri, Apr 01, 2016 at 03:31:17PM +0200, Dennis Kaarsemaker wrote:
>>> On wo, 2016-03-30 at 15:09 +0530, Karthik Nayak wrote:

 This is part of unification of the commands 'git tag -l, git branch -l
 and git for-each-ref'. This ports over branch.c to use ref-filter's
 printing options.

 Initially posted here: $(gmane/279226). It was decided that this series
 would follow up after refactoring ref-filter parsing mechanism, which
 is now merged into master (9606218b32344c5c756f7c29349d3845ef60b80c).
>>>
>>> Interaction between this series and something I've not yet been able to
>>> identify
>>
>> That someting is es/test-gpg-tags. Karthik, can you maybe squash this
>> fix in if you do another reroll?
>
> Yes, I sent the same patch to Karthik yesterday. Unfortunately, I didn't
> send it as a response to this email thread, so it may have been hard to
> spot on the mailing list. My bad. Sorry for wasting your time. :(
>
> ATB,
> Ramsay Jones
>

Thanks to both of you, it seems that I hadn't rebased my work on Eric's patches
($gmane/288371). Thanks for reporting, I'll fix and reroll.

-- 
Regards,
Karthik Nayak
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3] builtin/apply: handle parse_binary() failure

[Christian Couder]

On Fri, Apr 1, 2016 at 1:16 PM, Junio C Hamano  wrote:
> Christian Couder  writes:
>>
>> It looks like this patch is not in pu. Maybe it has fallen through the 
>> cracks?
>
> Yup, it indeed was ignored (giving priority to work towards 2.8
> during the freeze) and then was forgotten.
>
> My comment on the first one that exited mentions "your follow-up
> patch", but I cannot quite tell which one, as there was no threading
> in your original patches.  Does this change need to come before
> something else that I already have?

No, I think by "your follow-up patch" you meant the other one that had
also been forgotten and that I also replied to, that is:

[PATCH] builtin/apply: free patch when parse_chunk() fails

I had first sent both of them at around the same time.

Thanks,
Christian.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] submodule--helper, module_clone: always operate on absolute paths

[Junio C Hamano]

Junio C Hamano  writes:

> By the way, this line is the last use of sm_gitdir_rel before it
> gets freed.  I wonder
>
>   sm_gitdir = absolute_path(sb.buf);
>
> would be sufficient.  We can lose the variable, and free() on it at
> the end of this function, and an extra allocation if we can do so.
>
>> +if (!is_absolute_path(path)) {
>> +strbuf_addf(, "%s/%s", get_git_work_tree(), path);
>> +path = strbuf_detach(, 0);
>> +} else
>> +path = xstrdup(path);
>>  
>>  if (!file_exists(sm_gitdir)) {
>>  if (safe_create_leading_directories_const(sm_gitdir) < 0)
>
> Other than that, looks good to me.

Another thing I noticed is that it will not stay safe forever to
borrow the result from absolute_path() for extended period of time.
So how about this one (and Ramsay's "NULL must be spelled NULL, not
0") on top?

-- >8 --
From: Junio C Hamano 
Date: Fri, 1 Apr 2016 12:23:16 -0700
Subject: [PATCH] submodule--helper: do not borrow absolute_path() result for 
too long

absolute_path() is designed to allow its callers to take a brief
peek of the result (typically, to be fed to functions like
strbuf_add() and relative_path() as a parameter) without having to
worry about freeing it, but the other side of the coin of that
memory model is that the caller shouldn't rely too much on the
result living forever--there may be a helper function the caller
subsequently calls that makes its own call to absolute_path(),
invalidating the earlier result.

Use xstrdup() to make our own copy, and free(3) it when we are done.
While at it, remove an unnecessary sm_gitdir_rel variable that was
only used to as a parameter to call absolute_paht() and never used
again.

Signed-off-by: Junio C Hamano 
---
 builtin/submodule--helper.c | 8 +++-
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index b660a22..e69b340 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -157,8 +157,7 @@ static int module_clone(int argc, const char **argv, const 
char *prefix)
const char *reference = NULL, *depth = NULL;
int quiet = 0;
FILE *submodule_dot_git;
-   char *sm_gitdir_rel, *p, *path = NULL;
-   const char *sm_gitdir;
+   char *p, *path = NULL, *sm_gitdir;
struct strbuf rel_path = STRBUF_INIT;
struct strbuf sb = STRBUF_INIT;
 
@@ -199,8 +198,7 @@ static int module_clone(int argc, const char **argv, const 
char *prefix)
die(_("submodule--helper: unspecified or empty --path"));
 
strbuf_addf(, "%s/modules/%s", get_git_dir(), name);
-   sm_gitdir_rel = strbuf_detach(, NULL);
-   sm_gitdir = absolute_path(sm_gitdir_rel);
+   sm_gitdir = xstrdup(absolute_path(sb.buf));
 
if (!is_absolute_path(path)) {
strbuf_addf(, "%s/%s", get_git_work_tree(), path);
@@ -245,7 +243,7 @@ static int module_clone(int argc, const char **argv, const 
char *prefix)
   relative_path(path, sm_gitdir, _path));
strbuf_release();
strbuf_release(_path);
-   free(sm_gitdir_rel);
+   free(sm_gitdir);
free(path);
free(p);
return 0;
-- 
2.8.0-225-g297c00e

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] submodule--helper, module_clone: always operate on absolute paths

[Junio C Hamano]

Stefan Beller  writes:

> + char *sm_gitdir_rel, *p, *path = NULL;
> + const char *sm_gitdir;
>   struct strbuf rel_path = STRBUF_INIT;
>   struct strbuf sb = STRBUF_INIT;
>  
> @@ -198,7 +199,14 @@ static int module_clone(int argc, const char **argv, 
> const char *prefix)
>   die(_("submodule--helper: unspecified or empty --path"));
>  
>   strbuf_addf(, "%s/modules/%s", get_git_dir(), name);
> - sm_gitdir = strbuf_detach(, NULL);
> + sm_gitdir_rel = strbuf_detach(, NULL);
> + sm_gitdir = absolute_path(sm_gitdir_rel);

With this change, sm_gitdir will always be absolute, which means the
parameter to clone_submodule() call (immedately after this hunk)
will now be always absolute.  I do not think "git clone" called from
there will leave anything in the resulting repository that depends
on the --separate-git-dir= being relative or absolute, so this
change should not cause us new problems.

By the way, this line is the last use of sm_gitdir_rel before it
gets freed.  I wonder

sm_gitdir = absolute_path(sb.buf);

would be sufficient.  We can lose the variable, and free() on it at
the end of this function, and an extra allocation if we can do so.

> + if (!is_absolute_path(path)) {
> + strbuf_addf(, "%s/%s", get_git_work_tree(), path);
> + path = strbuf_detach(, 0);
> + } else
> + path = xstrdup(path);
>  
>   if (!file_exists(sm_gitdir)) {
>   if (safe_create_leading_directories_const(sm_gitdir) < 0)

Other than that, looks good to me.

Thanks.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: weird diff output?

[Junio C Hamano]

Stefan Beller  writes:

> Thanks for going through examples!
> (I would, too. But fixing a submodule regression is more important
> now; I only develop new features when there are no known regressions
> caused by me)

This is a tangent but perhaps as an experiment perhaps we can try it
as the rule for everybody to adopt for one cycle, and see if that
improves the quality of the end-user experience?
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 13/16] ref-filter: allow porcelain to translate messages in the output

[Karthik Nayak]

cc'ing Matthieu since this patch was initially written by him.

On Thu, Mar 31, 2016 at 3:28 AM, Junio C Hamano  wrote:
> Karthik Nayak  writes:
>
>> +static struct ref_msg {
>> + const char *gone;
>> + const char *ahead;
>> + const char *behind;
>> + const char *ahead_behind;
>> +} msgs = {
>> + "gone",
>> + "ahead %d",
>> + "behind %d",
>> + "ahead %d, behind %d"
>> +};
>> +
>> +void setup_ref_filter_porcelain_msg(void)
>> +{
>> + msgs.gone = _("gone");
>> + msgs.ahead = _("ahead %d");
>> + msgs.behind = _("behind %d");
>> + msgs.ahead_behind = _("ahead %d, behind %d");
>> +}
>
> I do not think this patch is wrong, but I wonder if it would be
> sufficient to have a single bit in file-scope static variable and
> flip it in setup_ref_filter_porcelain_msg().  I.e.
>
> static int use_porcelain_msg; /* false by default */
>
> void setup_ref_filter_porcelain_msg(void)
> {
> use_porcelain_msg = 1;
> }
>
> static const char *P_(const char *msg)
> {
> return use_porcelain_msg ? _(msg) : msg;
> }
>
> and then mark the message up perhaps like so:
>
> -   *s = xstrdup("gone");
> +   *s = xstrdup(P_("gone"));
>
> which may make things much simpler.
>
> We'd need to update Makefile to recognize X_() as another keyword;
> I'd think something like this should do:
>
>  XGETTEXT_FLAGS_C = $(XGETTEXT_FLAGS) --language=C \
> ---keyword=_ --keyword=N_ --keyword="Q_:1,2"
> +--keyword=_ --keyword=N_ --keyword=P_ --keyword="Q_:1,2"
>
>
>>  typedef enum { FIELD_STR, FIELD_ULONG, FIELD_TIME } cmp_type;
>>
>>  struct align {
>> @@ -1097,15 +1117,15 @@ static void fill_remote_ref_details(struct used_atom 
>> *atom, const char *refname,
>>   else if (atom->u.remote_ref.option == RR_TRACK) {
>>   if (stat_tracking_info(branch, _ours,
>>  _theirs, NULL)) {
>> - *s = xstrdup("gone");
>> + *s = xstrdup(msgs.gone);
>>   } else if (!num_ours && !num_theirs)
>>   *s = "";
>>   else if (!num_ours)
>> - *s = xstrfmt("behind %d", num_theirs);
>> + *s = xstrfmt(msgs.behind, num_theirs);
>>   else if (!num_theirs)
>> - *s = xstrfmt("ahead %d", num_ours);
>> + *s = xstrfmt(msgs.ahead, num_ours);
>>   else
>> - *s = xstrfmt("ahead %d, behind %d",
>> + *s = xstrfmt(msgs.ahead_behind,
>>num_ours, num_theirs);
>>   if (!atom->u.remote_ref.nobracket && *s[0]) {
>>   const char *to_free = *s;
>> diff --git a/ref-filter.h b/ref-filter.h
>> index 0014b92..da17145 100644
>> --- a/ref-filter.h
>> +++ b/ref-filter.h
>> @@ -111,5 +111,7 @@ struct ref_sorting *ref_default_sorting(void);
>>  int parse_opt_merge_filter(const struct option *opt, const char *arg, int 
>> unset);
>>  /*  Get the current HEAD's description */
>>  char *get_head_description(void);
>> +/*  Set up translated strings in the output. */
>> +void setup_ref_filter_porcelain_msg(void);
>>
>>  #endif /*  REF_FILTER_H  */

I'm not totally knowledgeable  about how porcelain works, although
Matthieu did give me a
brief explanation. I guess it'd better to hear his thoughts on this.
For me both the current patch
and your suggestion, seem good to go.

-- 
Regards,
Karthik Nayak
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 05/16] ref-filter: move get_head_description() from branch.c

[Karthik Nayak]

On Thu, Mar 31, 2016 at 3:46 AM, Junio C Hamano  wrote:
> Karthik Nayak  writes:
>
>> -static char *get_head_description(void)
>> -{
>> - struct strbuf desc = STRBUF_INIT;
>> - struct wt_status_state state;
>> - memset(, 0, sizeof(state));
>> - wt_status_get_state(, 1);
>> - if (state.rebase_in_progress ||
>> - state.rebase_interactive_in_progress)
>> - strbuf_addf(, _("(no branch, rebasing %s)"),
>> - state.branch);
>> - else if (state.bisect_in_progress)
>> - strbuf_addf(, _("(no branch, bisect started on %s)"),
>> - state.branch);
>> - else if (state.detached_from) {
>> - /* TRANSLATORS: make sure these match _("HEAD detached at ")
>> -and _("HEAD detached from ") in wt-status.c */
>> - if (state.detached_at)
>> - strbuf_addf(, _("(HEAD detached at %s)"),
>> - state.detached_from);
>> - else
>> - strbuf_addf(, _("(HEAD detached from %s)"),
>> - state.detached_from);
>> - }
>> - else
>> - strbuf_addstr(, _("(no branch)"));
>> - free(state.branch);
>> - free(state.onto);
>> - free(state.detached_from);
>> - return strbuf_detach(, NULL);
>> -}
>> -
>
> Hmph, the name used to be a good one within the context of
> implementation of "git branch" command, but I have to wonder if it
> is a specific enough name in the global context of the entire
> project.  I also wondered if this sits better in wt-status.c;
> doesn't "git status" do something similar?
>

Yes, `wt_status_print(struct wt_status *s)` pretty much does the same, maybe
we can eventually lose this in favor of using that.

> For now, this should do, as I do not think of anything better.
>
> Thanks.

Sure.

-- 
Regards,
Karthik Nayak
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 09/16] ref-filter: make "%(symref)" atom work with the ':short' modifier

[Karthik Nayak]

On Thu, Mar 31, 2016 at 3:47 AM, Junio C Hamano  wrote:
> Karthik Nayak  writes:
>
>> The "%(symref)" atom doesn't work when used with the ':short' modifier
>> because we strictly match only 'symref' for setting the 'need_symref'
>> indicator. Fix this by using comparing with valid_atom rather than used_atom.
>>
>> Add tests for %(symref) and %(symref:short) while we're here.
>>
>> Helped-by: Junio C Hamano 
>> Signed-off-by: Karthik Nayak 
>> ---
>>  ref-filter.c|  2 +-
>>  t/t6300-for-each-ref.sh | 24 
>>  2 files changed, 25 insertions(+), 1 deletion(-)
>>
>> diff --git a/ref-filter.c b/ref-filter.c
>> index 8c97cdb..5c59b17 100644
>> --- a/ref-filter.c
>> +++ b/ref-filter.c
>> @@ -338,7 +338,7 @@ int parse_ref_filter_atom(const char *atom, const char 
>> *ep)
>>   valid_atom[i].parser(_atom[at], arg);
>>   if (*atom == '*')
>>   need_tagged = 1;
>> - if (!strcmp(used_atom[at].name, "symref"))
>> + if (!strcmp(valid_atom[i].name, "symref"))
>>   need_symref = 1;
>>   return at;
>>  }
>
> Makes perfect sense.
>
>> diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh
>> index 2c5f177..b06ea1c 100755
>> --- a/t/t6300-for-each-ref.sh
>> +++ b/t/t6300-for-each-ref.sh
>> @@ -38,6 +38,7 @@ test_atom() {
>>   case "$1" in
>>   head) ref=refs/heads/master ;;
>>tag) ref=refs/tags/testtag ;;
>> +  sym) ref=refs/heads/sym ;;
>>  *) ref=$1 ;;
>>   esac
>
> An earlier review may have missed this, but I just noticed that the
> body of this case/esac is over-indented.  Something we can fix later
> after the dust settles.
>

Since I'll be re-rolling, I'll fix it :)

-- 
Regards,
Karthik Nayak
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/2] mergetools: add support for ExamDiff

[Junio C Hamano]

Jacob Nisnevich  writes:

> OK I add the quotes and modified the comment. I also changed $folder to 
> $sub_directory. I think that makes a little bit more sense and sounds a lot
> better.
>
> Jacob Nisnevich (2):
>   mergetools: create mergetool_find_win32_cmd() helper function for
> winmerge
>   mergetools: add support for ExamDiff
>
>  git-mergetool--lib.sh | 25 +
>  mergetools/examdiff   | 18 ++
>  mergetools/winmerge   | 21 +
>  3 files changed, 44 insertions(+), 20 deletions(-)
>  create mode 100644 mergetools/examdiff

This round looked good to me.
David, does this look sensible to you?

Thanks.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 00/21] replacement for dt/refs-backend-lmdb v7 patch 04/33

[David Turner]

On Thu, 2016-03-31 at 18:37 -0700, Stefan Beller wrote:
> On Wed, Mar 30, 2016 at 1:05 PM, David Turner <
> dtur...@twopensource.com> wrote:
> > On Wed, 2016-03-30 at 08:37 +0200, Michael Haggerty wrote:
> > > On 03/29/2016 10:12 PM, David Turner wrote:
> > > > On Sun, 2016-03-27 at 07:22 +0200, Michael Haggerty wrote:
> > > > > On 03/24/2016 07:47 AM, David Turner wrote:
> > > > > > [...]
> > > > > > I incorporated your changes into the lmdb backend.  To make
> > > > > > merging
> > > > > > later more convenient, I rebased on top of pu -- I think
> > > > > > this
> > > > > > mainly
> > > > > > depends on jk/check-repository-format, but I also included
> > > > > > some
> > > > > > fixes
> > > > > > for a couple of tests that had been changed by other
> > > > > > patches.
> > > > > 
> > > > > I think rebasing changes on top of pu is counterproductive. I
> > > > > believe
> > > > > that Junio had extra work rebasing your earlier series onto a
> > > > > merge
> > > > > of
> > > > > the minimum number of topics that it really depended on.
> > > > > There is
> > > > > no
> > > > > way
> > > > > that he could merge the branch in this form because it would
> > > > > imply
> > > > > merging all of pu.
> > > > > 
> > > > > See the zeroth section of SubmittingPatches [1] for the
> > > > > guidelines.
> > > > 
> > > > I'm a bit confused because
> > > > [PATCH 18/21] get_default_remote(): remove unneeded flag
> > > > variable
> > > > 
> > > > doesn't do anything on master -- it depends on some patch in
> > > > pu.
> > > >  And
> > > > we definitely want to pick up jk/check-repository-format (which
> > > > doesn't
> > > > include whatever 18/21 depends on).
> > > > 
> > > > So what do you think our base should be?
> > > 
> > > I think the preference is to base a patch series on the merge of
> > > master
> > > plus the minimum number of topics in pu (ideally, none) that are
> > > "essential" prerequisites of the changes in the patch series. For
> > > example, the version of this patch series that Junio has in his
> > > tree
> > > was
> > > based on master + sb/submodule-parallel-update.
> > > 
> > > Even if there are minor
> > > conflicts with another in-flight topic, it is easier for Junio to
> > > resolve the conflicts when merging the topics together than to
> > > rebase
> > > the patch series over and over as the other patch series evolves.
> > > The
> > > goal of this practice is of course to allow patch series to
> > > evolve
> > > independently of each other as much as possible.
> > > 
> > > Of course if you have insights into nontrivial conflicts between
> > > your
> > > patch series and others, it would be helpful to discuss these in
> > > your
> > > cover letter.
> > 
> > If I am reading this correctly, it looks like your series also has
> > a
> > few more sb submodule patches, e.g. sb/submodule-init, which is
> > responsible for the code that 18/21 depends on.
> > 
> > I think jk/check-repository-format is also  good to get in first,
> > because it changes the startup sequence a bit and it's a bit tricky
> > to
> > figure out what needs to change in dt/refs-backend-lmdb as a result
> > of
> > it.
> > 
> > But I can't just merge jk/check-repository-format on top of
> > 71defe0047
> > -- some function signatures have changed in the run-command stuff
> > and
> > it seems kind of annoying to fix up.
> > 
> > So I propose instead that we just drop 18/21 for now, and use just
> > jk/check-repository-format as the base.
> 
> By 18/21 you mean
> [PATCH 18/21] get_default_remote(): remove unneeded flag variable
> in builtin/submodule--helper.c?
> You could drop that and I'll pick it up in one of the submodule
> series',
> if that is more convenient for you.
> 


Yes, thanks.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv4 3/4] bundle: don't leak an fd in case of early return

[Jeff King]

On Fri, Apr 01, 2016 at 10:29:51AM -0700, Junio C Hamano wrote:

> > This does still suffer from the double-close I mentioned earlier. I'm
> > not sure if we want to address that or not.
> 
> Something like this on top?
> 
>  bundle.c | 7 +--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/bundle.c b/bundle.c
> index 08e32ca..bbf4efa 100644
> --- a/bundle.c
> +++ b/bundle.c
> @@ -453,8 +453,10 @@ int create_bundle(struct bundle_header *header, const 
> char *path,
>   goto err;
>  
>   /* write pack */
> - if (write_pack_data(bundle_fd, ))
> + if (write_pack_data(bundle_fd, )) {
> + bundle_fd = -1; /* already closed by the above call */
>   goto err;
> + }
>  
>   if (!bundle_to_stdout) {
>   if (commit_lock_file())
> @@ -463,7 +465,8 @@ int create_bundle(struct bundle_header *header, const 
> char *path,
>   return 0;
>  err:
>   if (!bundle_to_stdout) {
> - close(bundle_fd);
> + if (0 <= bundle_fd)
> + close(bundle_fd);
>   rollback_lock_file();

Yep, that looks fine.

-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv4 1/4] notes: don't leak memory in git_config_get_notes_strategy

[Eric Sunshine]

On Fri, Apr 1, 2016 at 1:14 PM, Jeff King  wrote:
> On Fri, Apr 01, 2016 at 10:03:25AM -0700, Junio C Hamano wrote:
>> From: Stefan Beller 
>> Date: Thu, 31 Mar 2016 11:04:03 -0700
>> Subject: [PATCH] notes: don't leak memory in git_config_get_notes_strategy
>>
>> This function asks for the value of a configuration and
>> after using the value does not have to retain ownership
>> of the value.  git_config_get_string_const() however is
>> a function to get a copy of the value, but we forget to
>> free it before we return.
>>
>> Because we only need to peek the value without retaining
>> a pointer to it, use git_config_get_value() to peek into
>> the value cached in the config API layer.
>>
>> As git_config_get_value() does not insist the value to be
>> a string, we'd need to do the "nonbool" check ourselves.

Nicer commit message.

> Unfortunately, I don't think this is quite right. In the original, we
> relied on git_config_get_string_const to notice a non-string value, at
> which point it would die:
>
>   $ git -c notes.mergeStrategy notes merge whatever
>   error: Missing value for 'notes.mergeStrategy'
>   fatal: unable to parse 'notes.mergeStrategy' from command-line config
>
> But in your patch:
>
>> + if (!value)
>> + return config_error_nonbool(key);
>
> We just return an error from git_config_get_notes_strategy(). If this
> were a callback to git_config(), that would be fine (as we would
> auto-die then in the caller), but it's not. It is called directly for a
> specific key. One of the callers treats a non-zero return as "we don't
> have that variable", and the other ignores the return value completely.
>
> So I think you'd want something more like:
>
>   if (!value) {
> config_error_nonbool(key);
> git_die_config(key);
>   }

Yep, I had noted the bit about having to die() manually when reviewing
the earlier patch, but it slipped from memory when composing the reply
to the current version of the patch.

> This is why I wondered if the minor "do not allocate" tweak was worth
> the trouble, when git_config_get_string() just handles this for us.

Which again suggests a new function which does this work but doesn't
copy the string (despite the already quite large set of similar
functions).
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv4 3/4] bundle: don't leak an fd in case of early return

[Junio C Hamano]

Jeff King  writes:

> You didn't see my response to the other patch yet. :)
>
>> >/* write pack */
>> >if (write_pack_data(bundle_fd, ))
>> > -  return -1;
>> > +  goto err;
>
> This does still suffer from the double-close I mentioned earlier. I'm
> not sure if we want to address that or not.

Something like this on top?

 bundle.c | 7 +--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/bundle.c b/bundle.c
index 08e32ca..bbf4efa 100644
--- a/bundle.c
+++ b/bundle.c
@@ -453,8 +453,10 @@ int create_bundle(struct bundle_header *header, const char 
*path,
goto err;
 
/* write pack */
-   if (write_pack_data(bundle_fd, ))
+   if (write_pack_data(bundle_fd, )) {
+   bundle_fd = -1; /* already closed by the above call */
goto err;
+   }
 
if (!bundle_to_stdout) {
if (commit_lock_file())
@@ -463,7 +465,8 @@ int create_bundle(struct bundle_header *header, const char 
*path,
return 0;
 err:
if (!bundle_to_stdout) {
-   close(bundle_fd);
+   if (0 <= bundle_fd)
+   close(bundle_fd);
rollback_lock_file();
}
return -1;
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: GIT_CONFIG - what's the point?

[SZEDER Gábor]

> Let me explain my scenario. I have an nfs mounted home directory. It
> is used across multiple machines. I use different colored xterms for
> each machine. But that means that the one set of colors in my one
> .gitconfig file don't work against all my screen backgrounds. I'm
> trying to find a way to tune the git colors per login. The ability to
> set colors in an environment variable (like most UNIX utils support)
> would be the easiest way to do this. Failing that, I was hoping that
> by setting GIT_CONFIG per login, I could tune the color schemes with
> different config files.
> 
> Since that is not how GIT_CONFIG is used, I have simply decided to
> squint where necessary, or open up a neutral colored xterm for the
> diff, regardless of machine.
> 
> Yes, I could probably do diffs in many other ways, but git diff at the
> command line is usually the most expedient.
> 
> Unless I wanted to define a GIT_CONFIG_OVER environment variable upon
> login, place inside it the appropriate -c= overrides for
> colors, and then define a bash function git as
> 
> git () {
>$(which git) $GIT_CONFIG_OVER "$@"
>return $?
> }
> 
> which seems silly.

Yeah, that 'return $?' at the end of the function does indeed seem
silly :)  (sorry, couldn't resist...)

You could use machine-specific config includes instead of that
GIT_CONFIG_OVER environment variable.  I.e. store machine-specific
color configuration in ~/.gitcolors. or something and define
the shell function as:

git () {
command git -c include.path=~/.gitcolors.$HOSTNAME "$@"
}

The impact on your .bashrc would be much smaller than with the
GIT_CONFIG_OVER approach.
You could even turn this into an alias, if you want.


--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] builtin/apply: free patch when parse_chunk() fails

[Junio C Hamano]

Christian Couder  writes:

> When parse_chunk() fails it can return -1, for example
> when find_header() doesn't find a patch header.
>
> In this case it's better in apply_patch() to free the
> "struct patch" that we just allocated instead of
> leaking it.
>
> Signed-off-by: Christian Couder 
> ---
>  builtin/apply.c | 4 +++-

OK.  Will queue.  Thanks.

>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/builtin/apply.c b/builtin/apply.c
> index 42c610e..bf78282 100644
> --- a/builtin/apply.c
> +++ b/builtin/apply.c
> @@ -4373,8 +4373,10 @@ static int apply_patch(int fd, const char *filename, 
> int options)
>   patch->inaccurate_eof = !!(options & INACCURATE_EOF);
>   patch->recount =  !!(options & RECOUNT);
>   nr = parse_chunk(buf.buf + offset, buf.len - offset, patch);
> - if (nr < 0)
> + if (nr < 0) {
> + free_patch(patch);
>   break;
> + }
>   if (apply_in_reverse)
>   reverse_patches(patch);
>   if (use_patch(patch)) {
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3] builtin/apply: handle parse_binary() failure

[Junio C Hamano]

Christian Couder  writes:

> On Fri, Mar 18, 2016 at 1:30 PM, Christian Couder
>  wrote:
>> In parse_binary() there is:
>>
>> forward = parse_binary_hunk(, , , );
>> if (!forward && !status)
>> /* there has to be one hunk (forward hunk) */
>> return error(_("unrecognized binary patch at line %d"), 
>> linenr-1);
>>
>> so parse_binary() can return -1, because that's what error() returns.
>>
>> Also parse_binary_hunk() sets "status" to -1 in case of error and
>> parse_binary() does "if (status) return status;".
>>
>> In this case parse_chunk() should not add -1 to the patchsize it computes.
>> It is better for future libification efforts to make it just return -1.
>>
>> Signed-off-by: Christian Couder 
>> ---
>> Only the title of the patch changed in this version compared to v2.
>
> It looks like this patch is not in pu. Maybe it has fallen through the cracks?

Yup, it indeed was ignored (giving priority to work towards 2.8
during the freeze) and then was forgotten.

My comment on the first one that exited mentions "your follow-up
patch", but I cannot quite tell which one, as there was no threading
in your original patches.  Does this change need to come before
something else that I already have?

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv4 3/4] bundle: don't leak an fd in case of early return

[Jeff King]

On Fri, Apr 01, 2016 at 10:05:49AM -0700, Junio C Hamano wrote:

> Stefan Beller  writes:
> 
> > In successful operation `write_pack_data` will close the `bundle_fd`,
> > but when we exit early, we need to take care of the file descriptor
> > as well as the lock file ourselves. The lock file may be deleted at the
> > end of running the program, but we are in library code, so we should
> > not rely on that.
> >
> > Helped-by: Jeff King 
> > Signed-off-by: Stefan Beller 
> > ---
> 
> Thanks.  I think this turned out to be the trickiest one to get
> right among the four and my reading of this round tells me that
> it addresses all the trickiness pointed out in the reviews.

You didn't see my response to the other patch yet. :)

> > /* write pack */
> > if (write_pack_data(bundle_fd, ))
> > -   return -1;
> > +   goto err;

This does still suffer from the double-close I mentioned earlier. I'm
not sure if we want to address that or not.

-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv4 1/4] notes: don't leak memory in git_config_get_notes_strategy

[Jeff King]

On Fri, Apr 01, 2016 at 10:03:25AM -0700, Junio C Hamano wrote:

> -- >8 --
> From: Stefan Beller 
> Date: Thu, 31 Mar 2016 11:04:03 -0700
> Subject: [PATCH] notes: don't leak memory in git_config_get_notes_strategy
> 
> This function asks for the value of a configuration and
> after using the value does not have to retain ownership
> of the value.  git_config_get_string_const() however is
> a function to get a copy of the value, but we forget to
> free it before we return.
> 
> Because we only need to peek the value without retaining
> a pointer to it, use git_config_get_value() to peek into
> the value cached in the config API layer.
> 
> As git_config_get_value() does not insist the value to be
> a string, we'd need to do the "nonbool" check ourselves.

Unfortunately, I don't think this is quite right. In the original, we
relied on git_config_get_string_const to notice a non-string value, at
which point it would die:

  $ git -c notes.mergeStrategy notes merge whatever
  error: Missing value for 'notes.mergeStrategy'
  fatal: unable to parse 'notes.mergeStrategy' from command-line config

But in your patch:

> @@ -743,8 +743,10 @@ static int git_config_get_notes_strategy(const char *key,
>  {
>   const char *value;
>  
> - if (git_config_get_string_const(key, ))
> + if (git_config_get_value(key, ))
>   return 1;
> + if (!value)
> + return config_error_nonbool(key);
>   if (parse_notes_merge_strategy(value, strategy))
>   git_die_config(key, "unknown notes merge strategy %s", value);

We just return an error from git_config_get_notes_strategy(). If this
were a callback to git_config(), that would be fine (as we would
auto-die then in the caller), but it's not. It is called directly for a
specific key. One of the callers treats a non-zero return as "we don't
have that variable", and the other ignores the return value completely.

So I think you'd want something more like:

  if (!value) {
config_error_nonbool(key);
git_die_config(key);
  }

That keeps the original message intact (though it is a bit verbose in
the first place).

This is why I wondered if the minor "do not allocate" tweak was worth
the trouble, when git_config_get_string() just handles this for us.

-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv4 3/4] bundle: don't leak an fd in case of early return

[Junio C Hamano]

Stefan Beller  writes:

> In successful operation `write_pack_data` will close the `bundle_fd`,
> but when we exit early, we need to take care of the file descriptor
> as well as the lock file ourselves. The lock file may be deleted at the
> end of running the program, but we are in library code, so we should
> not rely on that.
>
> Helped-by: Jeff King 
> Signed-off-by: Stefan Beller 
> ---

Thanks.  I think this turned out to be the trickiest one to get
right among the four and my reading of this round tells me that
it addresses all the trickiness pointed out in the reviews.

Will replace.

>  bundle.c | 18 +-
>  1 file changed, 13 insertions(+), 5 deletions(-)
>
> diff --git a/bundle.c b/bundle.c
> index 506ac49..08e32ca 100644
> --- a/bundle.c
> +++ b/bundle.c
> @@ -435,12 +435,14 @@ int create_bundle(struct bundle_header *header, const 
> char *path,
>  
>   /* write prerequisites */
>   if (compute_and_write_prerequisites(bundle_fd, , argc, argv))
> - return -1;
> + goto err;
>  
>   argc = setup_revisions(argc, argv, , NULL);
>  
> - if (argc > 1)
> - return error(_("unrecognized argument: %s"), argv[1]);
> + if (argc > 1) {
> + error(_("unrecognized argument: %s"), argv[1]);
> + goto err;
> + }
>  
>   object_array_remove_duplicates();
>  
> @@ -448,17 +450,23 @@ int create_bundle(struct bundle_header *header, const 
> char *path,
>   if (!ref_count)
>   die(_("Refusing to create empty bundle."));
>   else if (ref_count < 0)
> - return -1;
> + goto err;
>  
>   /* write pack */
>   if (write_pack_data(bundle_fd, ))
> - return -1;
> + goto err;
>  
>   if (!bundle_to_stdout) {
>   if (commit_lock_file())
>   die_errno(_("cannot create '%s'"), path);
>   }
>   return 0;
> +err:
> + if (!bundle_to_stdout) {
> + close(bundle_fd);
> + rollback_lock_file();
> + }
> + return -1;
>  }
>  
>  int unbundle(struct bundle_header *header, int bundle_fd, int flags)
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv4 1/4] notes: don't leak memory in git_config_get_notes_strategy

[Junio C Hamano]

Junio C Hamano  writes:

> Eric Sunshine  writes:
>
>> Meh. Rather than reverting the git_config_get_value(), it would have
>> been just as easy and safer (less chance of a future change
>> re-introducing a leak) if you had just inserted the necessary check
>> here:
>>
>> if (!value)
>> return  config_error_nonbool(key);
>
> Yup, sounds much more sensible fix that is useful in the longer term
> (and avoids one unnecessary strdup()).

Perhaps like this?

-- >8 --
From: Stefan Beller 
Date: Thu, 31 Mar 2016 11:04:03 -0700
Subject: [PATCH] notes: don't leak memory in git_config_get_notes_strategy

This function asks for the value of a configuration and
after using the value does not have to retain ownership
of the value.  git_config_get_string_const() however is
a function to get a copy of the value, but we forget to
free it before we return.

Because we only need to peek the value without retaining
a pointer to it, use git_config_get_value() to peek into
the value cached in the config API layer.

As git_config_get_value() does not insist the value to be
a string, we'd need to do the "nonbool" check ourselves.

Signed-off-by: Stefan Beller 
Signed-off-by: Junio C Hamano 
---
 builtin/notes.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/builtin/notes.c b/builtin/notes.c
index 52aa9af..c1265eb 100644
--- a/builtin/notes.c
+++ b/builtin/notes.c
@@ -743,8 +743,10 @@ static int git_config_get_notes_strategy(const char *key,
 {
const char *value;
 
-   if (git_config_get_string_const(key, ))
+   if (git_config_get_value(key, ))
return 1;
+   if (!value)
+   return config_error_nonbool(key);
if (parse_notes_merge_strategy(value, strategy))
git_die_config(key, "unknown notes merge strategy %s", value);
 
-- 
2.8.0-225-g297c00e

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] doc: clarify that notes can be attached to any type of stored object

[Junio C Hamano]

Junio C Hamano  writes:

> Sebastian Schuberth  writes:
>
>> Signed-off-by: Sebastian Schuberth 
>> ---
>>  Documentation/git-notes.txt | 5 +++--
>>  1 file changed, 3 insertions(+), 2 deletions(-)
>>
> I do not think this is correct place to patch.

So,... where is the right place?

Let's disect what we have in the DESCRIPTION section.

DESCRIPTION
---
Adds, removes, or reads notes attached to objects, without touching
the objects themselves.

This says "notes attached to objects" and "the objects themselves".
They do not limit the target of attaching a note to "commits".
So this may be the place to add "  Note that notes can be attached
to any kind of objects, not limited to commits" or something, if
we really wanted to.

By default, notes are saved to and read from `refs/notes/commits`, but
this default can be overridden.  See the OPTIONS, CONFIGURATION, and
ENVIRONMENT sections below.  If this ref does not exist, it will be
quietly created when it is first needed to store a note.

A typical use of notes is to supplement a commit message without
changing the commit itself. Notes can be shown by 'git log' along with
the original commit message. To distinguish these notes from the
message stored in the commit object, the notes are indented like the
message, after an unindented line saying "Notes ():" (or
"Notes:" for `refs/notes/commits`).

Notes can also be added to patches prepared with `git format-patch` by
using the `--notes` option. Such notes are added as a patch commentary
after a three dash separator line.

This paragraph _might_ be confusing to new readers.  The "added to"
sounds as if you are attaching a note to a non-object which is a
patch.  But this "add" is about "inserting the contents of the note
attached to the commit being formatted" and corresponds to "can be
shown by" in the previous paragraph.  We may want to avoid the verb
"add" when talking about the use of data stored in an existing note
to somewhere else like this.

To change which notes are shown by 'git log', see the
"notes.displayRef" configuration in linkgit:git-log[1].

See the "notes.rewrite." configuration for a way to carry
notes across commands that rewrite commits.



SUBCOMMANDS
---

list::
List the notes object for a given object. If no object is
given, show a list of all note objects and the objects they
annotate (in the format " ").
This is the default subcommand if no subcommand is given.

add::
Add notes for a given object (defaults to HEAD). Abort if the

And this "Add notes for " should probably be reworded to "Attach
notes to" to match the first sentence in the description.

object already has notes (use `-f` to overwrite existing notes).
However, if you're using `add` interactively (using an editor
to supply the notes contents), then - instead of aborting -
the existing notes will be opened in the editor (like the `edit`
subcommand).

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 3/4] format-patch: introduce --base=auto option

[Junio C Hamano]

Ye Xiaolong  writes:

> For the info starts with "base-commit:  ...", robot would know it
> is reliable base commit, it would just checkout it and apply the prerequisite
> patches and patchset for the work.
>
> For the info starts with "parent-commit: ; parent-patch-id: ",
> there are 3 situations 0day robot would need to handle:

It all depends on how you are computing this "parent-commit" thing.
When I reviewed the patch that implemented the fallback, my
impression was that it wasn't computing anything relevant, but just
was picking one of the random commit nearby in the history.

Assuming that it is not the case, and it is computing something
sensible, then I still do not see a point in marking these guessed
ones differently as "parent-commit" (not "base-commit").  After all,
a "base-commit" that was explicitly specified by the user can be
something that is inappropriate (e.g. the user may have thought the
base was a well-known one when his work was built on top of an early
draft of the patch that was later tweaked), so recipients of the
patch series need to be prepared to deal with a bogus base anyway.

> 2) parent commit is well-known in-flight patch, since 0day maintains the
>database of in-flight patches indexed by their patch-ids and
>commit-ids(of the git tree mentioned below), it also exports a git tree
>which holds all in-flight patches, where each patchset map to a new branch:
>
>https://github.com/0day-ci/linux/branches
>
>0day will find that patch in database by parent patch id, then do the
>checkout and apply work.

If a user starts from some solid base, applies a well-known
in-flight series, and builds his series on it and sends his series
out, and the base is identified by the patch-id of the last patch of
the in-flight series, then from that "parent patch-id" the robot can
find a commit that is the result of applying the same in-flight
series on top of some other solid base, as there is no coordination
between the user and 0day-ci system.  I would imagine that the
difference between the bases do matter--otherwise you wouldn't be
building this elaborate "base-commit" system that rejects the notion
that it is not sufficient for patches to textually apply cleanly and
insists that they have to be applied to exactly one known state.

So, I am not sure why you think "patch-id" is a good way to identify
the commit to be based on.

> In practice, most developers may not feed exact commit id by --base=
> regularly when using git format-patch, this "showing parent commit" solution
> could save developers' energy and reach a high coverage in the meantime.

So, while I understand the desire and wish, I am not convinced "as a
fallback, use parent patch-id and guess where to apply" is a good
apprach to make the wish come true.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv4 1/4] notes: don't leak memory in git_config_get_notes_strategy

[Junio C Hamano]

Eric Sunshine  writes:

> Meh. Rather than reverting the git_config_get_value(), it would have
> been just as easy and safer (less chance of a future change
> re-introducing a leak) if you had just inserted the necessary check
> here:
>
> if (!value)
> return  config_error_nonbool(key);

Yup, sounds much more sensible fix that is useful in the longer term
(and avoids one unnecessary strdup()).

>
> But, perhaps it's not worth the patch churn at this point...
>
>> if (parse_notes_merge_strategy(value, strategy))
>> git_die_config(key, "unknown notes merge strategy %s", 
>> value);
>>
>> +   free(value);
>> return 0;
>>  }
>>
>> --
>> 2.5.0.264.gc776916.dirty
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 6/7] correct blame for files commited with CRLF

[tboegi]

From: Torsten Bögershausen 

git blame reports lines as not "Not Committed Yet" when they have
CRLF in the index, CRLF in the worktree and e.g. core.autocrlf is true.

Since commit c48053 "new safer autocrlf handling", files that have CRLF
in the index are not normalized at commit when e.g. core.autocrl is set.

Whenever a CLRF conversion is needed (or any filter us set), load the
index temporally, before calling convert_to_git()

Signed-off-by: Torsten Bögershausen 
---
 builtin/blame.c   |  6 +-
 convert.c | 10 ++
 convert.h |  1 +
 t/t8003-blame-corner-cases.sh | 14 ++
 4 files changed, 30 insertions(+), 1 deletion(-)

diff --git a/builtin/blame.c b/builtin/blame.c
index e982fb8..a219068 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -2376,7 +2376,11 @@ static struct commit *fake_working_tree_commit(struct 
diff_options *opt,
if (strbuf_read(, 0, 0) < 0)
die_errno("failed to read from stdin");
}
-   convert_to_git(path, buf.buf, buf.len, , 0);
+   if (convert_needs_conversion(path)) {
+   read_cache();
+   convert_to_git(path, buf.buf, buf.len, , 0);
+   discard_cache();
+   }
origin->file.ptr = buf.buf;
origin->file.size = buf.len;
pretend_sha1_file(buf.buf, buf.len, OBJ_BLOB, origin->blob_sha1);
diff --git a/convert.c b/convert.c
index 4ed5d89..02c50da 100644
--- a/convert.c
+++ b/convert.c
@@ -903,6 +903,16 @@ const char *get_convert_attr_ascii(const char *path)
return "";
 }
 
+int convert_needs_conversion(const char *path)
+{
+   struct conv_attrs ca;
+
+   convert_attrs(, path);
+   if (ca.drv || ca.ident || ca.crlf_action != CRLF_BINARY)
+   return 1;
+   return 0;
+}
+
 int convert_to_git(const char *path, const char *src, size_t len,
struct strbuf *dst, enum safe_crlf checksafe)
 {
diff --git a/convert.h b/convert.h
index ccf436b..ffd9c32 100644
--- a/convert.h
+++ b/convert.h
@@ -35,6 +35,7 @@ extern enum eol core_eol;
 extern const char *get_cached_convert_stats_ascii(const char *path);
 extern const char *get_wt_convert_stats_ascii(const char *path);
 extern const char *get_convert_attr_ascii(const char *path);
+int convert_needs_conversion(const char *path);
 
 /* returns 1 if *dst was used */
 extern int convert_to_git(const char *path, const char *src, size_t len,
diff --git a/t/t8003-blame-corner-cases.sh b/t/t8003-blame-corner-cases.sh
index 6568429..a9b266f 100755
--- a/t/t8003-blame-corner-cases.sh
+++ b/t/t8003-blame-corner-cases.sh
@@ -212,4 +212,18 @@ test_expect_success 'blame file with CRLF attributes text' 
'
grep "A U Thor" actual
 '
 
+test_expect_success 'blame file with CRLF core.autocrlf=true' '
+   git config core.autocrlf false &&
+   printf "testcase\r\n" >crlfinrepo &&
+   >.gitattributes &&
+   git add crlfinrepo &&
+   git commit -m "add crlfinrepo" &&
+   git config core.autocrlf true &&
+   mv crlfinrepo tmp &&
+   git checkout crlfinrepo &&
+   rm tmp &&
+   git blame crlfinrepo >actual &&
+   grep "A U Thor" actual
+'
+
 test_done
-- 
2.8.0.rc2.2.g1a4d45a.dirty

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 4/7] t0027: TC for combined attributes

[tboegi]

From: Torsten Bögershausen 

Add more test cases for the not normalized files ("NNO").
The  "text" attribute is most important, use it as the first parameter.
"ident", if set, is the second paramater followed by the eol attribute.
The eol attribute overrides core.autocrlf, which overrides core.eol.

Use loops to test more combinations of attributes, like
"* text eol=crlf" or especially "*text=auto eol=crlf".

Currently "* text=auto eol=lf" does the same as "* text eol=lf", as the
eol attribute overrides "text=auto", this will change in future.

Signed-off-by: Torsten Bögershausen 
---
 t/t0027-auto-crlf.sh | 321 +--
 1 file changed, 158 insertions(+), 163 deletions(-)

diff --git a/t/t0027-auto-crlf.sh b/t/t0027-auto-crlf.sh
index f33962b..55f45d3 100755
--- a/t/t0027-auto-crlf.sh
+++ b/t/t0027-auto-crlf.sh
@@ -52,14 +52,17 @@ create_gitattributes () {
 create_NNO_files () {
for crlf in false true input
do
-   for attr in "" auto text -text lf crlf
+   for attr in "" auto text -text
do
-   pfx=NNO_${crlf}_attr_${attr} &&
-   cp CRLF_mix_LF ${pfx}_LF.txt &&
-   cp CRLF_mix_LF ${pfx}_CRLF.txt &&
-   cp CRLF_mix_LF ${pfx}_CRLF_mix_LF.txt &&
-   cp CRLF_mix_LF ${pfx}_LF_mix_CR.txt &&
-   cp CRLF_mix_LF ${pfx}_CRLF_nul.txt
+   for aeol in "" lf crlf
+   do
+   pfx=NNO_attr_${attr}_aeol_${aeol}_${crlf}
+   cp CRLF_mix_LF ${pfx}_LF.txt &&
+   cp CRLF_mix_LF ${pfx}_CRLF.txt &&
+   cp CRLF_mix_LF ${pfx}_CRLF_mix_LF.txt &&
+   cp CRLF_mix_LF ${pfx}_LF_mix_CR.txt &&
+   cp CRLF_mix_LF ${pfx}_CRLF_nul.txt
+   done
done
done
 }
@@ -100,16 +103,17 @@ commit_check_warn () {
 }
 
 commit_chk_wrnNNO () {
-   crlf=$1
-   attr=$2
-   lfwarn=$3
-   crlfwarn=$4
-   lfmixcrlf=$5
-   lfmixcr=$6
-   crlfnul=$7
-   pfx=NNO_${crlf}_attr_${attr}
+   attr=$1 ; shift
+   aeol=$1 ; shift
+   crlf=$1 ; shift
+   lfwarn=$1 ; shift
+   crlfwarn=$1 ; shift
+   lfmixcrlf=$1 ; shift
+   lfmixcr=$1 ; shift
+   crlfnul=$1 ; shift
+   pfx=NNO_attr_${attr}_aeol_${aeol}_${crlf}
#Commit files on top of existing file
-   create_gitattributes "$attr" &&
+   create_gitattributes "$attr" $aeol &&
for f in LF CRLF CRLF_mix_LF LF_mix_CR CRLF_nul
do
fname=${pfx}_$f.txt &&
@@ -121,19 +125,19 @@ commit_chk_wrnNNO () {
test_expect_success "commit NNO files crlf=$crlf attr=$attr LF" '
check_warning "$lfwarn" ${pfx}_LF.err
'
-   test_expect_success "commit NNO files crlf=$crlf attr=$attr CRLF" '
+   test_expect_success "commit NNO files attr=$attr aeol=$aeol crlf=$crlf 
CRLF" '
check_warning "$crlfwarn" ${pfx}_CRLF.err
'
 
-   test_expect_success "commit NNO files crlf=$crlf attr=$attr 
CRLF_mix_LF" '
+   test_expect_success "commit NNO files attr=$attr aeol=$aeol crlf=$crlf 
CRLF_mix_LF" '
check_warning "$lfmixcrlf" ${pfx}_CRLF_mix_LF.err
'
 
-   test_expect_success "commit NNO files crlf=$crlf attr=$attr LF_mix_cr" '
+   test_expect_success "commit NNO files attr=$attr aeol=$aeol crlf=$crlf 
LF_mix_cr" '
check_warning "$lfmixcr" ${pfx}_LF_mix_CR.err
'
 
-   test_expect_success "commit NNO files crlf=$crlf attr=$attr CRLF_nul" '
+   test_expect_success "commit NNO files attr=$attr aeol=$aeol crlf=$crlf 
CRLF_nul" '
check_warning "$crlfnul" ${pfx}_CRLF_nul.err
'
 }
@@ -162,6 +166,7 @@ stats_ascii () {
 
 # contruct the attr/ returned by git ls-files --eol
 # Take none (=empty), one or two args
+# convert.c: eol=XX overrides text=auto
 attr_ascii () {
case $1,$2 in
-text,*)   echo "-text" ;;
@@ -169,8 +174,8 @@ attr_ascii () {
text,lf)   echo "text eol=lf" ;;
text,crlf) echo "text eol=crlf" ;;
auto,) echo "text=auto" ;;
-   auto,lf)   echo "text=auto eol=lf" ;;
-   auto,crlf) echo "text=auto eol=crlf" ;;
+   auto,lf)   echo "text eol=lf" ;;
+   auto,crlf) echo "text eol=crlf" ;;
lf,)   echo "text eol=lf" ;;
crlf,) echo "text eol=crlf" ;;
,) echo "" ;;
@@ -195,28 +200,29 @@ check_files_in_repo () {
 }
 
 check_in_repo_NNO () {
-   crlf=$1
-   attr=$2
-   lfname=$3
-   crlfname=$4
-   lfmixcrlf=$5
-   lfmixcr=$6
-   crlfnul=$7
-   pfx=NNO_${crlf}_attr_${attr}_
-   test_expect_success "compare_files $lfname ${pfx}LF.txt" '
-   compare_files $lfname 

Re: [PATCH v3 3/4] format-patch: introduce --base=auto option

[Junio C Hamano]

Ye Xiaolong  writes:

> On Thu, Mar 31, 2016 at 10:43:48AM -0700, Junio C Hamano wrote:
>>Xiaolong Ye  writes:
>>
>>> Introduce --base=auto to record the base commit info automatically, the 
>>> base_commit
>>> will be the merge base of tip commit of the upstream branch and 
>>> revision-range
>>> specified in cmdline.
>>
>>This line is probably a bit too long.
>
> How about simplifying it to "the base_commit is the merge base of upstream and
> specified revision-range."?

What I meant was not that profound.  I just wanted you to wrap your
lines a bit shorter so that quoting in the discussion thread like
this would not make the result overlong to fit on a 80-column
terminal ;-)

>>> +   base = base_list->item;
>>> +   free_commit_list(base_list);
>>
>>What should happen when there are multiple merge bases?  The code
>>picks one at random and ignores the remainder, if I am reading this
>>correctly.
>
> If there is more than one merge base, commits in base_list should
> be sorted by date, if I am understanding it correctly, so
> base_list->item should be the lastest merge base commit, it should
> be enough for us to used as base commit.

By definition, when there are multiple merge bases, there is no
latest one among them.

When the history involves criss-cross merges, there can be more than
one 'best' common ancestor for two commits.  For example, with this
topology (note that X is not a commit; it merely denotes crossing of
two lines):

   ---1---o---A
  /\ /
  ---O  X
  \/ \
   ---2---o---o---B

both '1' and '2' are merge-bases of 'A' and 'B'.  And the timestamps
on one (be it committer or author timestamp) being later than those
of the other do not make it any more suitable than the other one.

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 7/7] convert.c: more safer crlf handling with text attribute

[tboegi]

From: Torsten Bögershausen 

A follow-up after a discussion how to fix the flaky execution
of t0025, gmane/$284352.

This patch extends the work done in commit c480539:
"Make it work also for un-normalized repositories". Make sure that CRLF
can be converted round trip, or don't convert them at all.

The old handling would treat a file as unchanged after checkout,
as long as it is not touched in the work tree and mtime matches the value
recorded in the index.
When the mtime is changed in the working tree, or the inode is changed,
the file is reported as modified.

The following sequence is now handled reproducable:
$ git init
$ printf "line1\r\n" >file.bat
$ git add file.bat
$ git commit -m "Add file with CRLF" file.bat
$ echo "*.bat text eol=crlf" >.gitattributes
$ git commit -m "bat files should have CRLF"
$ git status
 # nothing to commit, working directory clean
$ git push 
$ printf "newline\r\n" >>file.bat
$ mv file.bat file.sav
$ git checkout file.bat
$ git status
 #modified:   file.bat

The new handling makes sure that after running "git reset --hard".
"git status" reports the working tree as clean regarding CRLF conversion.
It makes sure that the Git-internal eol conversion is
doing roundtrip. A user can still write an external smudge/clean filter
outside Git, which doesn't do a roundtrip and the working directory is
not clean.

The functionality of has_cr_in_index() is turned into has_crlf_in_index(),
and the function is integrated into would_convert_crlf_at_commit().

Check for CRLF in the index instead of CR, the bit CONVERT_STAT_BITS_ANY_CR
is no longer used and removed, as well as "lonecr" in struct text_stat.

Rewrite check_safe_crlf() in convert.c to simulate checkin-checkout,
to detect whether any line endings are converted.

Add a warning, similar to the CRLF-LF replacement, when a file is commited,
and after the next checkout the line endings are not they should be.

Modify the lf_to_crlf_filter:
Files with LF are converted into CRLF, file with CRLF are not changed.
Files with mixed line endings are not converted, the filter fails, and Git
falls back to the non-streaming handling, see write_entry().

Signed-off-by: Torsten Bögershausen 
---
 Documentation/gitattributes.txt |  16 ++-
 convert.c   | 232 +---
 t/t0025-crlf-auto.sh|   8 +-
 t/t0027-auto-crlf.sh|  58 +-
 4 files changed, 191 insertions(+), 123 deletions(-)

diff --git a/Documentation/gitattributes.txt b/Documentation/gitattributes.txt
index c2663c7..3caeb52 100644
--- a/Documentation/gitattributes.txt
+++ b/Documentation/gitattributes.txt
@@ -110,7 +110,7 @@ repository upon 'git add' and 'git commit'.
 `text`
 ^^
 
-This attribute enables and controls end-of-line normalization.  When a
+This attribute enables and controls end-of-line conversion.  When a
 text file is normalized, its line endings are converted to LF in the
 repository.  To control what line ending style is used in the working
 directory, use the `eol` attribute for a single file and the
@@ -120,8 +120,11 @@ Note that `core.autocrlf` overrides `core.eol`
 Set::
 
Setting the `text` attribute on a path enables end-of-line
-   normalization and marks the path as a text file.  End-of-line
+   conversion and marks the path as a text file.  End-of-line
conversion takes place without guessing the content type.
+   Files that have been commited with CRLF before the text attribute
+   is set and commited are not normalized. No end-of-line conversion
+   is done at checkout or checkin.
 
 Unset::
 
@@ -132,7 +135,8 @@ Set to string value "auto"::
 
When `text` is set to "auto", the path is marked for automatic
end-of-line normalization.  If Git decides that the content is
-   text, its line endings are normalized to LF on checkin.
+   text, and the path has no CRLF in the index,
+   its line endings are converted to LF on checkin.
 
 Unspecified::
 
@@ -147,8 +151,10 @@ unspecified.
 ^
 
 This attribute sets a specific line-ending style to be used in the
-working directory.  It sets the `text` attribute,
-unless `text=auto` is specified.
+working directory.  It sets the `text` attribute, unless `text=auto`
+is specified.
+When the file had been commited with CRLF in the index, no conversion
+is done at checkout or commit.
 
 Set to string value "crlf"::
 
diff --git a/convert.c b/convert.c
index 02c50da..1fddbe8 100644
--- a/convert.c
+++ b/convert.c
@@ -17,7 +17,8 @@
 #define CONVERT_STAT_BITS_TXT_LF0x1
 #define CONVERT_STAT_BITS_TXT_CRLF  0x2
 #define CONVERT_STAT_BITS_BIN   0x4
-#define CONVERT_STAT_BITS_ANY_CR0x8
+
+#define CONVERT_STAT_BITS_MIXED (CONVERT_STAT_BITS_TXT_LF | 
CONVERT_STAT_BITS_TXT_CRLF)
 
 enum crlf_action {
CRLF_UNDEFINED,
@@ -32,7 +33,7 @@ enum crlf_action {
 
 struct text_stat {
/* NUL, CR, LF and CRLF counts */
-   unsigned 

[PATCH v2 5/7] CRLF: unify the "auto" handling

[tboegi]

From: Torsten Bögershausen 

Make .gitattributes "* text=auto eol=crlf" to do the same as
setting core.autocrlf=true and "* text=auto eol=crlf" the same
as core.autocrlf=input

Signed-off-by: Torsten Bögershausen 
---
 Documentation/config.txt| 10 -
 Documentation/gitattributes.txt | 11 +-
 convert.c   | 37 -
 t/t0025-crlf-auto.sh|  4 ++--
 t/t0027-auto-crlf.sh| 45 +++--
 5 files changed, 53 insertions(+), 54 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 4a27ad4..dfaf39c 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -389,13 +389,13 @@ file with mixed line endings would be reported by the 
`core.safecrlf`
 mechanism.
 
 core.autocrlf::
-   Setting this variable to "true" is almost the same as setting
-   the `text` attribute to "auto" on all files except that text
-   files are not guaranteed to be normalized: files that contain
+   Setting this variable to "true" is the same as setting
+   the attributes to "auto eol=crlf" on all files.
+   Files are not guaranteed to be normalized: files that contain
`CRLF` in the repository will not be touched.  Use this
setting if you want to have `CRLF` line endings in your
-   working directory even though the repository does not have
-   normalized line endings.  This variable can be set to 'input',
+   working directory and the repository has normalized line endings.
+   This variable can be set to 'input',
in which case no output conversion is performed.
 
 core.symlinks::
diff --git a/Documentation/gitattributes.txt b/Documentation/gitattributes.txt
index e3b1de8..c2663c7 100644
--- a/Documentation/gitattributes.txt
+++ b/Documentation/gitattributes.txt
@@ -115,6 +115,7 @@ text file is normalized, its line endings are converted to 
LF in the
 repository.  To control what line ending style is used in the working
 directory, use the `eol` attribute for a single file and the
 `core.eol` configuration variable for all text files.
+Note that `core.autocrlf` overrides `core.eol`
 
 Set::
 
@@ -146,8 +147,8 @@ unspecified.
 ^
 
 This attribute sets a specific line-ending style to be used in the
-working directory.  It enables end-of-line normalization without any
-content checks, effectively setting the `text` attribute.
+working directory.  It sets the `text` attribute,
+unless `text=auto` is specified.
 
 Set to string value "crlf"::
 
@@ -187,8 +188,8 @@ regardless of their content.
 
 
 *.txt  text
-*.vcproj   eol=crlf
-*.sh   eol=lf
+*.vcproj   text eol=crlf
+*.sh   text eol=lf
 *.jpg  -text
 
 
@@ -198,7 +199,7 @@ normalization in Git.
 
 If you simply want to have CRLF line endings in your working directory
 regardless of the repository you are working with, you can set the
-config variable "core.autocrlf" without changing any attributes.
+config variable "core.autocrlf" without using any attributes.
 
 
 [core]
diff --git a/convert.c b/convert.c
index b6da114..4ed5d89 100644
--- a/convert.c
+++ b/convert.c
@@ -229,7 +229,9 @@ static enum eol output_eol(enum crlf_action crlf_action)
return EOL_LF;
case CRLF_UNDEFINED:
case CRLF_AUTO_CRLF:
+   return EOL_CRLF;
case CRLF_AUTO_INPUT:
+   return EOL_LF;
case CRLF_TEXT:
case CRLF_AUTO:
/* fall through */
@@ -302,15 +304,12 @@ static int crlf_to_git(const char *path, const char *src, 
size_t len,
if (crlf_action == CRLF_AUTO || crlf_action == CRLF_AUTO_INPUT || 
crlf_action == CRLF_AUTO_CRLF) {
if (stats.stat_bits & CONVERT_STAT_BITS_BIN)
return 0;
-
-   if (crlf_action == CRLF_AUTO_INPUT || crlf_action == 
CRLF_AUTO_CRLF) {
-   /*
-* If the file in the index has any CR in it, do not 
convert.
-* This is the new safer autocrlf handling.
-*/
-   if (has_cr_in_index(path))
-   return 0;
-   }
+   /*
+* If the file in the index has any CR in it, do not convert.
+* This is the new safer autocrlf handling.
+*/
+   if (has_cr_in_index(path))
+   return 0;
}
 
check_safe_crlf(path, crlf_action, , checksafe);
@@ -370,12 +369,10 @@ static int crlf_to_worktree(const char *path, const char 
*src, size_t len,
return 0;
 
if (crlf_action == CRLF_AUTO || crlf_action == CRLF_AUTO_INPUT || 
crlf_action == CRLF_AUTO_CRLF) {
-   if (crlf_action == CRLF_AUTO_INPUT || crlf_action == 

[PATCH v2 2/7] convert.c: stream and early out

[tboegi]

From: Torsten Bögershausen 

When statistics are done for the autocrlf handling, the search in
the content can be stopped, if e.g
- a search for binary is done, and a NUL character is found
- a search for CRLF is done, and the first CRLF is found.

Similar when statistics for binary vs non-binary are gathered:
Whenever a lone CR or NUL is found, the search can be aborted.

When checking out files in "auto" mode, any file that has a "lone CR"
or a CRLF will not be converted, so the search can be aborted early.

Add the new bit, CONVERT_STAT_BITS_ANY_CR,
which is set for either lone CR or CRLF.

Many binary files have a NUL very early (within the first few bytes,
latest within the first 1..2K).
It is often not necessary to load the whole content of a file or blob
into memory.

Use a streaming handling for blobs and files in the worktree.

Signed-off-by: Torsten Bögershausen 
---
 convert.c | 162 --
 1 file changed, 106 insertions(+), 56 deletions(-)

diff --git a/convert.c b/convert.c
index f524b8d..b6da114 100644
--- a/convert.c
+++ b/convert.c
@@ -3,6 +3,7 @@
 #include "run-command.h"
 #include "quote.h"
 #include "sigchain.h"
+#include "streaming.h"
 
 /*
  * convert.c - convert a file when checking it out and checking it in.
@@ -13,10 +14,10 @@
  * translation when the "text" attribute or "auto_crlf" option is set.
  */
 
-/* Stat bits: When BIN is set, the txt bits are unset */
 #define CONVERT_STAT_BITS_TXT_LF0x1
 #define CONVERT_STAT_BITS_TXT_CRLF  0x2
 #define CONVERT_STAT_BITS_BIN   0x4
+#define CONVERT_STAT_BITS_ANY_CR0x8
 
 enum crlf_action {
CRLF_UNDEFINED,
@@ -31,30 +32,36 @@ enum crlf_action {
 
 struct text_stat {
/* NUL, CR, LF and CRLF counts */
-   unsigned nul, lonecr, lonelf, crlf;
+   unsigned stat_bits, lonecr, lonelf, crlf;
 
/* These are just approximations! */
unsigned printable, nonprintable;
 };
 
-static void gather_stats(const char *buf, unsigned long size, struct text_stat 
*stats)
+static void do_gather_stats(const char *buf, unsigned long size,
+   struct text_stat *stats, unsigned earlyout)
 {
unsigned long i;
 
-   memset(stats, 0, sizeof(*stats));
-
+   if (!buf || !size)
+   return;
for (i = 0; i < size; i++) {
unsigned char c = buf[i];
if (c == '\r') {
+   stats->stat_bits |= CONVERT_STAT_BITS_ANY_CR;
if (i+1 < size && buf[i+1] == '\n') {
stats->crlf++;
i++;
-   } else
+   stats->stat_bits |= CONVERT_STAT_BITS_TXT_CRLF;
+   } else {
stats->lonecr++;
+   stats->stat_bits |= CONVERT_STAT_BITS_BIN;
+   }
continue;
}
if (c == '\n') {
stats->lonelf++;
+   stats->stat_bits |= CONVERT_STAT_BITS_TXT_LF;
continue;
}
if (c == 127)
@@ -67,7 +74,7 @@ static void gather_stats(const char *buf, unsigned long size, 
struct text_stat *
stats->printable++;
break;
case 0:
-   stats->nul++;
+   stats->stat_bits |= CONVERT_STAT_BITS_BIN;
/* fall through */
default:
stats->nonprintable++;
@@ -75,6 +82,8 @@ static void gather_stats(const char *buf, unsigned long size, 
struct text_stat *
}
else
stats->printable++;
+   if (stats->stat_bits & earlyout)
+   break; /* We found what we have been searching for */
}
 
/* If file ends with EOF then don't count this EOF as non-printable. */
@@ -86,41 +95,63 @@ static void gather_stats(const char *buf, unsigned long 
size, struct text_stat *
  * The same heuristics as diff.c::mmfile_is_binary()
  * We treat files with bare CR as binary
  */
-static int convert_is_binary(unsigned long size, const struct text_stat *stats)
+static void convert_nonprintable(struct text_stat *stats)
 {
-   if (stats->lonecr)
-   return 1;
-   if (stats->nul)
-   return 1;
if ((stats->printable >> 7) < stats->nonprintable)
-   return 1;
-   return 0;
+   stats->stat_bits |= CONVERT_STAT_BITS_BIN;
+}
+
+static void gather_stats(const char *buf, unsigned long size,
+struct text_stat *stats, unsigned earlyout)
+{
+   memset(stats, 0, sizeof(*stats));
+   do_gather_stats(buf, size, stats, earlyout);
+   

[PATCH v2 1/7] Make it possible to get sha1 for a path from the index

[tboegi]

From: Torsten Bögershausen 

Factor out the retrieval of the sha1 for a given path in
read_blob_data_from_index() into the function get_sha1_from_index().

This will be used in the next commit, when convert.c can do the analyze
for "text=auto" without slurping the whole blob into memory at once.

Add a wrapper definition get_sha1_from_cache().

Signed-off-by: Torsten Bögershausen 
- Changes against v1:
  Fixed typos, thanks Eric
  Corrected gitattributes.txt, "auto" will no longer normalize
  Rework of 7/7: Show the eol mismatch to the user when committing
---
 cache.h  |  3 +++
 read-cache.c | 29 ++---
 2 files changed, 21 insertions(+), 11 deletions(-)

diff --git a/cache.h b/cache.h
index b829410..bd1210a 100644
--- a/cache.h
+++ b/cache.h
@@ -379,6 +379,7 @@ extern void free_name_hash(struct index_state *istate);
 #define unmerge_cache_entry_at(at) unmerge_index_entry_at(_index, at)
 #define unmerge_cache(pathspec) unmerge_index(_index, pathspec)
 #define read_blob_data_from_cache(path, sz) 
read_blob_data_from_index(_index, (path), (sz))
+#define get_sha1_from_cache(path)  get_sha1_from_index (_index, (path))
 #endif
 
 enum object_type {
@@ -1008,6 +1009,8 @@ static inline void *read_sha1_file(const unsigned char 
*sha1, enum object_type *
return read_sha1_file_extended(sha1, type, size, LOOKUP_REPLACE_OBJECT);
 }
 
+const unsigned char *get_sha1_from_index(struct index_state *istate, const 
char *path);
+
 /*
  * This internal function is only declared here for the benefit of
  * lookup_replace_object().  Please do not call it directly.
diff --git a/read-cache.c b/read-cache.c
index d9fb78b..a3ef967 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -2263,13 +2263,27 @@ int index_name_is_other(const struct index_state 
*istate, const char *name,
 
 void *read_blob_data_from_index(struct index_state *istate, const char *path, 
unsigned long *size)
 {
-   int pos, len;
+   const unsigned char *sha1;
unsigned long sz;
enum object_type type;
void *data;
 
-   len = strlen(path);
-   pos = index_name_pos(istate, path, len);
+   sha1 = get_sha1_from_index(istate, path);
+   if (!sha1)
+   return NULL;
+   data = read_sha1_file(sha1, , );
+   if (!data || type != OBJ_BLOB) {
+   free(data);
+   return NULL;
+   }
+   if (size)
+   *size = sz;
+   return data;
+}
+
+const unsigned char *get_sha1_from_index(struct index_state *istate, const 
char *path)
+{
+   int pos = index_name_pos(istate, path, strlen(path));
if (pos < 0) {
/*
 * We might be in the middle of a merge, in which
@@ -2285,14 +2299,7 @@ void *read_blob_data_from_index(struct index_state 
*istate, const char *path, un
}
if (pos < 0)
return NULL;
-   data = read_sha1_file(istate->cache[pos]->sha1, , );
-   if (!data || type != OBJ_BLOB) {
-   free(data);
-   return NULL;
-   }
-   if (size)
-   *size = sz;
-   return data;
+   return (istate->cache[pos]->sha1);
 }
 
 void stat_validity_clear(struct stat_validity *sv)
-- 
2.8.0.rc2.2.g1a4d45a.dirty

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 3/7] Allow core.autocrlf=input and core.eol=crlf

[tboegi]

From: Torsten Bögershausen 

Commit  942e77 "Add "core.eol" config variable" adds a condition to
the config parser: core.autocrlf=input is not allowed together with
core.eol=crlf.

This may lead to unnecessary problems, when config files are parsed:
A global config file sets core.autocrlf=input,
the repo local config file sets is own configuration: core.eol=crlf

There is no need to prevent combinations in config.c, in any case
core.autocrlf overrides core.eol.
Allow all combinations of core.crlf and core.eol and document
that core.autocrlf overrides core.eol.

Signed-off-by: Torsten Bögershausen 
---
 Documentation/config.txt | 6 +++---
 config.c | 4 
 2 files changed, 3 insertions(+), 7 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 2cd6bdd..4a27ad4 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -337,9 +337,9 @@ core.quotePath::
 
 core.eol::
Sets the line ending type to use in the working directory for
-   files that have the `text` property set.  Alternatives are
-   'lf', 'crlf' and 'native', which uses the platform's native
-   line ending.  The default value is `native`.  See
+   files that have the `text` property set when core.autocrlf is false.
+   Alternatives are 'lf', 'crlf' and 'native', which uses the platform's
+   native line ending.  The default value is `native`.  See
linkgit:gitattributes[5] for more information on end-of-line
conversion.
 
diff --git a/config.c b/config.c
index 9ba40bc..a6adc8b 100644
--- a/config.c
+++ b/config.c
@@ -803,8 +803,6 @@ static int git_default_core_config(const char *var, const 
char *value)
 
if (!strcmp(var, "core.autocrlf")) {
if (value && !strcasecmp(value, "input")) {
-   if (core_eol == EOL_CRLF)
-   return error("core.autocrlf=input conflicts 
with core.eol=crlf");
auto_crlf = AUTO_CRLF_INPUT;
return 0;
}
@@ -830,8 +828,6 @@ static int git_default_core_config(const char *var, const 
char *value)
core_eol = EOL_NATIVE;
else
core_eol = EOL_UNSET;
-   if (core_eol == EOL_CRLF && auto_crlf == AUTO_CRLF_INPUT)
-   return error("core.autocrlf=input conflicts with 
core.eol=crlf");
return 0;
}
 
-- 
2.8.0.rc2.2.g1a4d45a.dirty

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 2/4] format-patch: add '--base' option to record base tree info

[Junio C Hamano]

Ye Xiaolong  writes:

> On Thu, Mar 31, 2016 at 10:38:04AM -0700, Junio C Hamano wrote:
>
>>The contents of this look OK, but does it format correctly via
>>AsciiDoc?  I suspect that only the first paragraph up to "of this
>>shape:" would appear correctly and all the rest would become funny.
>
> Sorry, just heard of AsciiDoc, I will try to use it to do the right format 
> work.

Please make sure "make -C Documentation" produces sensible output
for *.1 (manpage) and *.html.

>>> +   init_revisions(, NULL);
>>> +   revs.max_parents = 1;
>>> +   base->object.flags |= UNINTERESTING;
>>> +   add_pending_object(, >object, "base");
>>> +   for (i = 0; i < total; i++) {
>>> +   list[i]->object.flags |= 0;
>>
>>What does this statement do, exactly?  Are you clearing some bits
>>but not others, and if so which ones?
>
> My mistake, it's useless and should be removed.

It probably make sense to do "&= ~UNINTERESTING" there, though.  You
are adding one UNINTERESTING object (i.e. the base) and adding
objects that are on the list[] as interesting.

>>This shows the patches in the order discovered by the revision
>>traversal, which typically is newer to older.  Is that intended?
>>Is it assumed that the order of the patches does not matter?
>
> The prerequisite patches should show in topological order, thus robot
> could parse them one by one and apply the patches in reverse order.

If you have history where base is B, with three prerequisites 1-2-3,
before the patch series A-B-C, i.e.

B---1---2---3---A---B---C

if you are showing "base-commit: B" as the first line in the base
tree information block, it would be natural to expect that the
prerequisite patch ids are listed for 1 and then 2 and then finally
3, i.e.

base-commit: B
prerequisite-patch-id: 1
prerequisite-patch-id: 2
prerequisite-patch-id: 3

no?

Also I know _you_ intend to consume this by robot, but it makes me
wonder if with a minimum update you can make the output also more
useful for bystander humans.  A mailing list participant may

 - see an early round of a series that interests her,
 - try to apply them to her tree,
 - find that the series does not apply, but
 - sees that a block to help identify to what tree the series is
   meant to apply.

With a list of 40-hex alone, she may not be able to figure out the
prerequisites, but if there is some other clue that helps her to
identify the base commit and these patches, she may be able to
construct a tree that is close enough.  Maybe you can help her by
appending the title of the commit and patches at the end of these
lines?

This is not a strong suggestion (yet); I am thinking aloud at this
point, without knowing how much it would help in practice to do so.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] imap-send.c: implements the GIT_CURL_DEBUG environment variable

[Junio C Hamano]

Elia Pinto  writes:

> Implements the GIT_CURL_DEBUG environment variable to allow a greater
> degree of detail of GIT_CURL_VERBOSE, in particular the complete
> transport header and all the data payload exchanged.
> It might be useful if a particular situation could require a more
> thorough debugging analysis.

My impression is that using GIT_TRACE_* is the more mainstream
trend, and it may be beneficial to work any new debugging aid like
this one to fit within that mechanism.

I am not saying new GIT_*_DEBUG is wrong.  I just wanted to make
sure you have considered doing this as a new trace in GIT_TRACE_*
family and rejected that apporach with a very good reason, in
which case that rationale deserves to be in the log message.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] doc: clarify that notes can be attached to any type of stored object

[Junio C Hamano]

Sebastian Schuberth  writes:

> Signed-off-by: Sebastian Schuberth 
> ---
>  Documentation/git-notes.txt | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/git-notes.txt b/Documentation/git-notes.txt
> index 8de3499..5375d98 100644
> --- a/Documentation/git-notes.txt
> +++ b/Documentation/git-notes.txt
> @@ -234,8 +234,9 @@ which operation triggered the update, and the commit 
> authorship is
>  determined according to the usual rules (see linkgit:git-commit[1]).
>  These details may change in the future.
>  
> -It is also permitted for a notes ref to point directly to a tree
> -object, in which case the history of the notes can be read with
> +It is also permitted for a notes ref to point to any other object in
> +the object store besides commit objects, that is annotated tags, blobs
> +or trees. For the latter, the history of the notes can be read with
>  `git log -p -g `.

I do not think this is correct place to patch.  The original is not
talking about what objects can have notes attached at all.  What it
explains is this.

 aka refs/notes/ (where  typically is
"commit") is usually a commit, whose tree is a notes-shaped
tree.  The (normal) history you get by following the parent link
of the commit represents how the entire set of notes evolved.
However, it is OK for the  to point directly to a tree,
which is a notes-shaped one, without an enclosing commit.  You
would lose the normal way to learn how the entire set of notes
evolved, but you could do "git log -p -g ", i.e. by
following its reflog, to pretend as if the history is recorded.

There is no way a blob can be pointed by  there and expect
it to work sensibly at all.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 00/16] port branch.c to use ref-filter's printing options

[Ramsay Jones]

On 01/04/16 14:44, Dennis Kaarsemaker wrote:
> On Fri, Apr 01, 2016 at 03:31:17PM +0200, Dennis Kaarsemaker wrote:
>> On wo, 2016-03-30 at 15:09 +0530, Karthik Nayak wrote:
>>>
>>> This is part of unification of the commands 'git tag -l, git branch -l
>>> and git for-each-ref'. This ports over branch.c to use ref-filter's
>>> printing options.
>>>
>>> Initially posted here: $(gmane/279226). It was decided that this series
>>> would follow up after refactoring ref-filter parsing mechanism, which
>>> is now merged into master (9606218b32344c5c756f7c29349d3845ef60b80c).
>>
>> Interaction between this series and something I've not yet been able to
>> identify
> 
> That someting is es/test-gpg-tags. Karthik, can you maybe squash this
> fix in if you do another reroll?

Yes, I sent the same patch to Karthik yesterday. Unfortunately, I didn't
send it as a response to this email thread, so it may have been hard to
spot on the mailing list. My bad. Sorry for wasting your time. :(

ATB,
Ramsay Jones


> 
> diff --git a/t/t6302-for-each-ref-filter.sh b/t/t6302-for-each-ref-filter.sh
> index 98a1c49..7420e48 100755
> --- a/t/t6302-for-each-ref-filter.sh
> +++ b/t/t6302-for-each-ref-filter.sh
> @@ -349,6 +349,8 @@ test_expect_success 'check %(if)...%(then)...%(end) 
> atoms' '
>   A U Thor: refs/heads/side
>   A U Thor: refs/odd/spot
>  
> +
> +
>   A U Thor: refs/tags/foo1.10
>   A U Thor: refs/tags/foo1.3
>   A U Thor: refs/tags/foo1.6
> @@ -367,7 +369,9 @@ test_expect_success 'check 
> %(if)...%(then)...%(else)...%(end) atoms' '
>   A U Thor: refs/heads/master
>   A U Thor: refs/heads/side
>   A U Thor: refs/odd/spot
> - No author: refs/tags/double-tag
> + No author: refs/tags/annotated-tag
> + No author: refs/tags/doubly-annotated-tag
> + No author: refs/tags/doubly-signed-tag
>   A U Thor: refs/tags/foo1.10
>   A U Thor: refs/tags/foo1.3
>   A U Thor: refs/tags/foo1.6
> @@ -385,7 +389,9 @@ test_expect_success 'ignore spaces in %(if) atom usage' '
>   master: Head ref
>   side: Not Head ref
>   odd/spot: Not Head ref
> - double-tag: Not Head ref
> + annotated-tag: Not Head ref
> + doubly-annotated-tag: Not Head ref
> + doubly-signed-tag: Not Head ref
>   foo1.10: Not Head ref
>   foo1.3: Not Head ref
>   foo1.6: Not Head ref
> 
> 
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] http.c: implements the GIT_CURL_DEBUG environment variable

[Ramsay Jones]

On 01/04/16 11:44, Elia Pinto wrote:
> Implements the GIT_CURL_DEBUG environment variable to allow a greater
> degree of detail of GIT_CURL_VERBOSE, in particular the complete
> transport header and all the data payload exchanged.
> It might be useful if a particular situation could require a more
> thorough debugging analysis.
> 
> Signed-off-by: Elia Pinto 
> ---
>  http.c | 97 
> +-
>  1 file changed, 96 insertions(+), 1 deletion(-)
> 
> diff --git a/http.c b/http.c
> index dfc53c1..079779d 100644
> --- a/http.c
> +++ b/http.c
[snip]

> @@ -532,7 +623,11 @@ static CURL *get_curl_handle(void)
>   "your curl version is too old (>= 7.19.4)");
>  #endif
>  
> - if (getenv("GIT_CURL_VERBOSE"))
> + if (getenv("GIT_CURL_DEBUG")) {
> + curl_easy_setopt(result, CURLOPT_VERBOSE, 1);
> + curl_easy_setopt(result, CURLOPT_DEBUGFUNCTION, curl_trace);
> + curl_easy_setopt(result, CURLOPT_DEBUGDATA, NULL);
> + } else if (getenv("GIT_CURL_VERBOSE"))
>   curl_easy_setopt(result, CURLOPT_VERBOSE, 1);
>  
>   curl_easy_setopt(result, CURLOPT_USERAGENT,
> 

Again, maybe something like:

if (getenv("GIT_CURL_VERBOSE")) {
curl_easy_setopt(result, CURLOPT_VERBOSE, 1);
if (getenv("GIT_CURL_DEBUG"))
curl_easy_setopt(result, CURLOPT_DEBUGFUNCTION, curl_trace);
}

Although that does make GIT_CURL_DEBUG subordinate to GIT_CURL_VERBOSE.
So, that may not be desired ...

ATB,
Ramsay Jones


--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] imap-send.c: implements the GIT_CURL_DEBUG environment variable

[Ramsay Jones]

On 01/04/16 11:44, Elia Pinto wrote:
> Implements the GIT_CURL_DEBUG environment variable to allow a greater
> degree of detail of GIT_CURL_VERBOSE, in particular the complete
> transport header and all the data payload exchanged.
> It might be useful if a particular situation could require a more
> thorough debugging analysis.
> 
> Signed-off-by: Elia Pinto 
> ---
>  imap-send.c | 99 
> +++--
>  1 file changed, 97 insertions(+), 2 deletions(-)
> 
> diff --git a/imap-send.c b/imap-send.c
> index 4d3b773..cf79e7f 100644
> --- a/imap-send.c
> +++ b/imap-send.c
[snip]

> @@ -1442,8 +1532,13 @@ static CURL *setup_curl(struct imap_server_conf *srvc)
>  
>   curl_easy_setopt(curl, CURLOPT_UPLOAD, 1L);
>  
> - if (0 < verbosity || getenv("GIT_CURL_VERBOSE"))
> - curl_easy_setopt(curl, CURLOPT_VERBOSE, 1L);
> + if (0 < verbosity )

previously it was sufficient to set GIT_CURL_VERBOSE, now I have to
set verbosity too?

[Does it matter that you change "1L" to "1" in the curl_easy_setopt()
call? In http.c (line 567) it also uses "1", but ...]

> + if (getenv("GIT_CURL_DEBUG")) {
> + curl_easy_setopt(curl, CURLOPT_VERBOSE, 1);
> + curl_easy_setopt(curl, CURLOPT_DEBUGFUNCTION, 
> curl_trace);
> + } else if (getenv("GIT_CURL_VERBOSE"))
> + curl_easy_setopt(curl, CURLOPT_VERBOSE, 1);
> +
>  
>   return curl;
>  }
> 

I would have expected something like:

if (0 < verbosity || getenv("GIT_CURL_VERBOSE")) {
curl_easy_setopt(curl, CURLOPT_VERBOSE, 1L);
if (getenv("GIT_CURL_DEBUG"))
curl_easy_setopt(curl, CURLOPT_DEBUGFUNCTION, curl_trace);
}

Hope That Helps.

ATB,
Ramsay Jones



--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: GIT_CONFIG - what's the point?

[Jeff King]

On Fri, Apr 01, 2016 at 10:31:12AM -0400, Matthew Persico wrote:

> Let me explain my scenario. I have an nfs mounted home directory. It
> is used across multiple machines. I use different colored xterms for
> each machine. But that means that the one set of colors in my one
> .gitconfig file don't work against all my screen backgrounds. I'm
> trying to find a way to tune the git colors per login. The ability to
> set colors in an environment variable (like most UNIX utils support)
> would be the easiest way to do this. Failing that, I was hoping that
> by setting GIT_CONFIG per login, I could tune the color schemes with
> different config files.
> 
> Since that is not how GIT_CONFIG is used, I have simply decided to
> squint where necessary, or open up a neutral colored xterm for the
> diff, regardless of machine.
> 
> Yes, I could probably do diffs in many other ways, but git diff at the
> command line is usually the most expedient.

I think per-environment config is a reasonable thing to want. I can
think of three approaches with varying drawbacks:

  1. The cleanest thing would be for git's config-include directive to
 respect environment variables somehow. We do expand $HOME in

   [include]
   path = ~/.whatever

 but only $HOME (and if you're willing to set $HOME, you can just
 point to a different ~/.gitconfig in the first place).

 So the drawback is that it doesn't currently work, and would need a
 patch to git. ;)

  2. If you don't want to change $HOME, you might be OK with changing
 $XDG_CONFIG_HOME, which could then point to your machine-specific
 user-level config (and could use an include to pull in the common
 bits). See "git help config" for more details (particularly, I
 think the XDG source only kicks in if you have no ~/.gitconfig, but
 I might be misremembering the details).

 The drawback is that other things besides git use $XDG_CONFIG_HOME,
 so you might be affecting them.

 You could probably come up with some elaborate scheme where each
 host has its own $XDG_CONFIG_HOME, and you symlink in the common
 bits. Or something. But that gets complicated pretty fast.

  3. The "git -c" mechanism communicates config to sub-processes via the
 $GIT_CONFIG_PARAMETERS variable. Its format and even existence are
 undocumented, but something like this would probably do what you
 want:

   export GIT_CONFIG_PARAMETERS="'config1=value1' 'config2=value2'"

 The drawback is that it is definitely not officially supported.
 However, I don't think there any plans to get rid of it (and if
 anything, I'd suspect in the future the parser may get less picky,
 and it might become an officially supported interface; but don't
 quote me on that).

-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv3 0/2] Fix relative path issues in recursive submodules.

[Ramsay Jones]

On 01/04/16 01:17, Stefan Beller wrote:
> Thanks Junio for review!
> 
> v3:
>  * This is a resend of the last two patches of the series, i.e. it replaces
>44859d6626d4 and efdef1e2e in sb/submodule-helper-clone-regression-fix
>   
>  * use absolute_path for sm_gitdir

Hi Stefan,

In response to v1 of this series, I sent you a fixup patch to suppress a
sparse warning (submodule: don't use an integer as a NULL pointer, 21-02-2016).

In v2, you introduced a second identical warning (rather, for the same
reason: using 0 as a NULL pointer as the second argument to strbuf_detach()).

I was just about to send another patch, when you sent this out. As a result,
you have suppressed the new warning, but the original remains.

So, ...

>  * removed todo
>  * we need to free the intermediate sm_gitdir, so rename that to sm_gitdir_rel
>and free it afterwards.
>  * while we currently do not support an absolute `path`, we eventually might.
>If `path` is absolute it would be a pointer to `argv`, which would lead to 
> a
>crash. Duplicate the path and the crash is prevented.
>  (* I thought we could use it as well for `path`, but we cannot; as 
>get_git_work_tree() != cwd)
>  * diff to sb/submodule-helper-clone-regression-fix:
> 
> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> index 89cbbda..be7bf5f 100644
> --- a/builtin/submodule--helper.c
> +++ b/builtin/submodule--helper.c
> @@ -153,12 +153,13 @@ static int clone_submodule(const char *path, const char 
> *gitdir, const char *url
>  
>  static int module_clone(int argc, const char **argv, const char *prefix)
>  {
> - const char *path = NULL, *name = NULL, *url = NULL;
> + const char *name = NULL, *url = NULL;
>   const char *reference = NULL, *depth = NULL;
>   int quiet = 0;
>   FILE *submodule_dot_git;
> - char *sm_gitdir, *p;
> - struct strbuf rel_path = STRBUF_INIT; /* for relative_path */
> + char *sm_gitdir_rel, *p, *path = NULL;
> + const char *sm_gitdir;
> + struct strbuf rel_path = STRBUF_INIT;
>   struct strbuf sb = STRBUF_INIT;
>  
>   struct option module_clone_options[] = {
> @@ -198,26 +199,14 @@ static int module_clone(int argc, const char **argv, 
> const char *prefix)
>   die(_("submodule--helper: unspecified or empty --path"));
>  
>   strbuf_addf(, "%s/modules/%s", get_git_dir(), name);
> - sm_gitdir = strbuf_detach(, NULL);
> -
> -
> - if (!is_absolute_path(sm_gitdir)) {
> - char *cwd = xgetcwd();
> - strbuf_addf(, "%s/%s", cwd, sm_gitdir);
> - sm_gitdir = strbuf_detach(, 0);
> - free(cwd);
> - }
> + sm_gitdir_rel = strbuf_detach(, NULL);

... this is good, but ...

> + sm_gitdir = absolute_path(sm_gitdir_rel);
>  
>   if (!is_absolute_path(path)) {
> - /*
> -  * TODO: add prefix here once we allow calling from non root
> -  * directory?
> -  */
> - strbuf_addf(, "%s/%s",
> - get_git_work_tree(),
> - path);
> + strbuf_addf(, "%s/%s", get_git_work_tree(), path);
>   path = strbuf_detach(, 0);

... can you please fix this up.

Thanks!

ATB,
Ramsay Jones


> - }
> + } else
> + path = xstrdup(path);
>  
>   if (!file_exists(sm_gitdir)) {
>   if (safe_create_leading_directories_const(sm_gitdir) < 0)
> @@ -240,6 +229,7 @@ static int module_clone(int argc, const char **argv, 
> const char *prefix)
>   submodule_dot_git = fopen(sb.buf, "w");
>   if (!submodule_dot_git)
>   die_errno(_("cannot open file '%s'"), sb.buf);
> +
>   fprintf_or_die(submodule_dot_git, "gitdir: %s\n",
>  relative_path(sm_gitdir, path, _path));
>   if (fclose(submodule_dot_git))
> @@ -255,8 +245,8 @@ static int module_clone(int argc, const char **argv, 
> const char *prefix)
>  relative_path(path, sm_gitdir, _path));
>   strbuf_release();
>   strbuf_release(_path);
> - free(sm_gitdir);
> -
> + free(sm_gitdir_rel);
> + free(path);
>   free(p);
>   return 0;
>  }
> 
> v2:
>  * reworded commit message for test (Thanks Junio!)
>  * instead of "simplifying" the path handling in patch 2, we are prepared
>for anything the user throws at us (i.e. instead of segfault we
>die(_("submodule--helper: unspecified or empty --path"));
>(Thanks Eric, Jacob for pushing back here!)
>  * reword commit message for patch 3 (safe_create_leading_directories_const is
>not a check, Thanks Junio!)
>  * patch 4 (the fix): That got reworked completely. No flow controlling
>conditions in the write out phase!
>  * patch 5 is a bonus! (replace fprintf by fprintf_or die) When implementing
>that I wondered if we also have close_or_die and open_or_die, but that 
> doesn't
>seem to be the case.
> 
> Thanks,
> Stefan
> 
> v1:
> 
> It took 

Re: GIT_CONFIG - what's the point?

[Matthew Persico]

Let me explain my scenario. I have an nfs mounted home directory. It
is used across multiple machines. I use different colored xterms for
each machine. But that means that the one set of colors in my one
.gitconfig file don't work against all my screen backgrounds. I'm
trying to find a way to tune the git colors per login. The ability to
set colors in an environment variable (like most UNIX utils support)
would be the easiest way to do this. Failing that, I was hoping that
by setting GIT_CONFIG per login, I could tune the color schemes with
different config files.

Since that is not how GIT_CONFIG is used, I have simply decided to
squint where necessary, or open up a neutral colored xterm for the
diff, regardless of machine.

Yes, I could probably do diffs in many other ways, but git diff at the
command line is usually the most expedient.

Unless I wanted to define a GIT_CONFIG_OVER environment variable upon
login, place inside it the appropriate -c= overrides for
colors, and then define a bash function git as

git () {
   $(which git) $GIT_CONFIG_OVER "$@"
   return $?
}

which seems silly.

Thanks anyway.

On Fri, Apr 1, 2016 at 8:38 AM, Jeff King  wrote:
> On Thu, Mar 31, 2016 at 08:54:26PM -0400, Matthew Persico wrote:
>
>> So, what's the point of GIT_CONFIG if only git-config uses it? Or did
>> I miss a step?
>
> There isn't a point to it. It's historical cruft that has been left in
> to avoid breaking older scripts. The same thing is generally better
> accomplished by using git-config's "--file" parameter. We should
> probably do a better job of making that clear in the documentation.
>
> Or possibly deprecate it and eventually remove it entirely, as discussed
> in:
>
>   
> http://thread.gmane.org/gmane.linux.debian.devel.bugs.general/1195694/focus=257770
>
> -Peff



-- 
Matthew O. Persico
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Signed-off-by vs Reviewed-by

[Miklos Vajna]

Hi,

On Thu, Mar 31, 2016 at 09:28:44AM -0700, Junio C Hamano  
wrote:
> The internal "parse the existing trailer block and manipulate it by
> adding, conditionally adding, replacing and deleting it" logic was
> done as an experimental "interpret-trailers" program, but polishing
> it (both its design and implementation) and integrating it to the
> front-line programs (e.g. "git commit") hasn't been done.

I had a look at interpret-trailers, and one use-case I miss is: being
able to define a trailer type, but only add it when asked explicitly.

Example:


$ git config trailer.review.key "Reviewed-by: "
$ git config trailer.review.command 'echo "$(git config user.name) <$(git 
config user.email)>"'
$ echo foo|git interpret-trailers
foo

Reviewed-by: A U Thor 
$ echo foo|git interpret-trailers --trailer review
foo

Reviewed-by: A U Thor 


I can imagine e.g. a new configuration vaulue named
trailer..ifMissing explicit, and when that's set, the trailer
would be only added if it's spelled out explicitly using '--trailer
'.

Does this sound like a good idea, or did I miss some way how this is
already possible? :-)

> As to the last step of "integration", we cannot use short-and-sweet
> single letter options like '-s' (for sign-off) for each and every
> custom trailer different projects use for their own purpose (as
> there are only 26 of the lowercase ASCII alphabet letters), so the
> most general syntax for the option has to become "--trailer "
> or some variation of it, and at that point "-s" would look like a
> short-hand for "--trailer signed-off-by".

Hmm, I think the above has to be implemented first, otherwise it'll be
hard to make "-s" an alias of "--trailer signed-off-by". (I mean having
git understand what "signed-off-by" is, still adding it conditionally.)

Regards,

Miklos


signature.asc
Description: Digital signature

Re: [PATCH v3 3/4] format-patch: introduce --base=auto option

[Ye Xiaolong]

On Thu, Mar 31, 2016 at 10:43:48AM -0700, Junio C Hamano wrote:
>Xiaolong Ye  writes:
>
>> Introduce --base=auto to record the base commit info automatically, the 
>> base_commit
>> will be the merge base of tip commit of the upstream branch and 
>> revision-range
>> specified in cmdline.
>
>This line is probably a bit too long.

How about simplifying it to "the base_commit is the merge base of upstream and
specified revision-range."?

>
>>
>> Helped-by: Junio C Hamano 
>> Helped-by: Wu Fengguang 
>> Signed-off-by: Xiaolong Ye 
>> ---
>>  Documentation/git-format-patch.txt |  4 
>>  builtin/log.c  | 31 +++
>>  2 files changed, 31 insertions(+), 4 deletions(-)
>>
>> diff --git a/Documentation/git-format-patch.txt 
>> b/Documentation/git-format-patch.txt
>> index 067d562..d8fe651 100644
>> --- a/Documentation/git-format-patch.txt
>> +++ b/Documentation/git-format-patch.txt
>> @@ -290,6 +290,10 @@ you can use `--suffix=-patch` to get 
>> `0001-description-of-my-change-patch`.
>>  patches for A, B and C, and the identifiers for P, X, Y, Z are appended
>>  at the end of the _first_ message.
>>  
>> +If set '--base=auto' in cmdline, it will track base commit 
>> automatically,
>> +the base commit will be the merge base of tip commit of the 
>> remote-tracking
>> +branch and revision-range specified in cmdline.
>> +
>>  --root::
>>  Treat the revision argument as a , even if it
>>  is just a single commit (that would normally be treated as a
>> diff --git a/builtin/log.c b/builtin/log.c
>> index 03cbab0..c5efe73 100644
>> --- a/builtin/log.c
>> +++ b/builtin/log.c
>> @@ -1200,6 +1200,9 @@ static void prepare_bases(struct base_tree_info *bases,
>>  struct rev_info revs;
>>  struct diff_options diffopt;
>>  struct object_id *patch_id;
>> +struct branch *curr_branch;
>> +struct commit_list *base_list;
>> +const char *upstream;
>>  unsigned char sha1[20];
>>  int i;
>>  
>> @@ -1207,10 +1210,30 @@ static void prepare_bases(struct base_tree_info 
>> *bases,
>>  DIFF_OPT_SET(, RECURSIVE);
>>  diff_setup_done();
>>  
>> -base = lookup_commit_reference_by_name(base_commit);
>> -if (!base)
>> -die(_("Unknown commit %s"), base_commit);
>> -oidcpy(>base_commit, >object.oid);
>> +if (!strcmp(base_commit, "auto")) {
>> +curr_branch = branch_get(NULL);
>
>Can branch_get() return NULL?  Which ...
>
>> +upstream = branch_get_upstream(curr_branch, NULL);
>
>... would cause branch_get_upstream() to give you an error (which
>you ignore)?  I guess that is OK because upstream will safely be set
>to NULL in that case.
>
Yes, branch_get_upstream(curr_branch, NULL) will safely return NULL if 
curr_branch
is NULL, so I think there is no need to add error handling for branch_get().

>> +if (upstream) {
>> +if (get_sha1(upstream, sha1))
>> +die(_("Failed to resolve '%s' as a valid 
>> ref."), upstream);
>> +commit = lookup_commit_or_die(sha1, "upstream base");
>> +base_list = get_merge_bases_many(commit, total, list);
>> +if (!bases)
>> +die(_("Could not find merge base."));
>> +base = base_list->item;
>> +free_commit_list(base_list);
>
>What should happen when there are multiple merge bases?  The code
>picks one at random and ignores the remainder, if I am reading this
>correctly.

If there is more than one merge base, commits in base_list should be sorted by 
date,
if I am understanding it correctly, so base_list->item should be the lastest
merge base commit, it should be enough for us to used as base commit.

Thanks,
Xiaolong.
>
>> +oidcpy(>base_commit, >object.oid);
>> +} else {
>> +die(_("Failed to get upstream, if you want to record 
>> base commit automatically,\n"
>> +  "please use git branch --set-upstream-to to track 
>> a remote branch.\n"
>> +  "Or you could specify base commit by 
>> --base= manually."));
>> +}
>> +} else {
>> +base = lookup_commit_reference_by_name(base_commit);
>> +if (!base)
>> +die(_("Unknown commit %s"), base_commit);
>> +oidcpy(>base_commit, >object.oid);
>> +}
>>  
>>  init_revisions(, NULL);
>>  revs.max_parents = 1;
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 00/16] port branch.c to use ref-filter's printing options

[Dennis Kaarsemaker]

On Fri, Apr 01, 2016 at 03:31:17PM +0200, Dennis Kaarsemaker wrote:
> On wo, 2016-03-30 at 15:09 +0530, Karthik Nayak wrote:
> > 
> > This is part of unification of the commands 'git tag -l, git branch -l
> > and git for-each-ref'. This ports over branch.c to use ref-filter's
> > printing options.
> > 
> > Initially posted here: $(gmane/279226). It was decided that this series
> > would follow up after refactoring ref-filter parsing mechanism, which
> > is now merged into master (9606218b32344c5c756f7c29349d3845ef60b80c).
> 
> Interaction between this series and something I've not yet been able to
> identify

That someting is es/test-gpg-tags. Karthik, can you maybe squash this
fix in if you do another reroll?

diff --git a/t/t6302-for-each-ref-filter.sh b/t/t6302-for-each-ref-filter.sh
index 98a1c49..7420e48 100755
--- a/t/t6302-for-each-ref-filter.sh
+++ b/t/t6302-for-each-ref-filter.sh
@@ -349,6 +349,8 @@ test_expect_success 'check %(if)...%(then)...%(end) atoms' '
A U Thor: refs/heads/side
A U Thor: refs/odd/spot
 
+
+
A U Thor: refs/tags/foo1.10
A U Thor: refs/tags/foo1.3
A U Thor: refs/tags/foo1.6
@@ -367,7 +369,9 @@ test_expect_success 'check 
%(if)...%(then)...%(else)...%(end) atoms' '
A U Thor: refs/heads/master
A U Thor: refs/heads/side
A U Thor: refs/odd/spot
-   No author: refs/tags/double-tag
+   No author: refs/tags/annotated-tag
+   No author: refs/tags/doubly-annotated-tag
+   No author: refs/tags/doubly-signed-tag
A U Thor: refs/tags/foo1.10
A U Thor: refs/tags/foo1.3
A U Thor: refs/tags/foo1.6
@@ -385,7 +389,9 @@ test_expect_success 'ignore spaces in %(if) atom usage' '
master: Head ref
side: Not Head ref
odd/spot: Not Head ref
-   double-tag: Not Head ref
+   annotated-tag: Not Head ref
+   doubly-annotated-tag: Not Head ref
+   doubly-signed-tag: Not Head ref
foo1.10: Not Head ref
foo1.3: Not Head ref
foo1.6: Not Head ref


-- 
Dennis Kaarsemaker 
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 2/4] format-patch: add '--base' option to record base tree info

[Ye Xiaolong]

On Thu, Mar 31, 2016 at 10:38:04AM -0700, Junio C Hamano wrote:
>Xiaolong Ye  writes:
>
>> Maintainers or third party testers may want to know the exact base tree
>> the patch series applies to. Teach git format-patch a '--base' option to
>> record the base tree info and append this information at the end of the
>> _first_ message (either the cover letter or the first patch in the series).
>
>You'd need a description of what "base tree info" consists of as a
>separate paragraph after the above paragraph.  I'd also suggest to
>
>   s/and append this information/and append it/;
>
>Based on my understanding of what you consider "base tree info", it
>may look like this, but you know your design better, so I'd expect
>you to rewrite it to be more useful, or at least to fill in the
>blanks.
>
>   The base tree info consists of the "base commit", which is a
>   well-known commit that is part of the stable part of the
>   project history everybody else works off of, and zero or
>   more "prerequisite patches", which are well-known patches in
>   flight that is not yet part of the "base commit" that need
>   to be applied on top of "base commit" ???IN WHAT ORDER???
>   before the patches can be applied.
>
>   "base commit" is shown as "base-commit: " followed by the
>   40-hex of the commit object name.  A "prerequisite patch" is
>   shown as "prerequisite-patch-id: " followed by the 40-hex
>   "patch id", which can be obtained by ???DOING WHAT???
>
Thanks for the review.

Ok, I'll polish up the description of base tree info and add it to commit log
as you suggested.

>> Helped-by: Junio C Hamano 
>> Helped-by: Wu Fengguang 
>> Signed-off-by: Xiaolong Ye 
>> ---
>>  Documentation/git-format-patch.txt | 25 +++
>>  builtin/log.c  | 89 
>> ++
>>  2 files changed, 114 insertions(+)
>>
>> diff --git a/Documentation/git-format-patch.txt 
>> b/Documentation/git-format-patch.txt
>> index 6821441..067d562 100644
>> --- a/Documentation/git-format-patch.txt
>> +++ b/Documentation/git-format-patch.txt
>> @@ -265,6 +265,31 @@ you can use `--suffix=-patch` to get 
>> `0001-description-of-my-change-patch`.
>>Output an all-zero hash in each patch's From header instead
>>of the hash of the commit.
>>  
>> +--base=::
>> +Record the base tree information to identify the whole tree
>> +the patch series applies to. For example, the patch submitter
>> +has a commit history of this shape:
>> +
>> +---P---X---Y---Z---A---B---C
>> +
>> +where "P" is the well-known public commit (e.g. one in Linus's tree),
>> +"X", "Y", "Z" are prerequisite patches in flight, and "A", "B", "C"
>> +are the work being sent out, the submitter could say "git format-patch
>> +--base=P -3 C" (or variants thereof, e.g. with "--cover" or using
>> +"Z..C" instead of "-3 C" to specify the range), and the identifiers
>> +for P, X, Y, Z are appended at the end of the _first_ message (either
>> +the cover letter or the first patch in the series).
>> +
>> +For non-linear topology, such as
>> +
>> +---P---X---A---M---C
>> +\ /
>> + Y---Z---B
>> +
>> +the submitter could also use "git format-patch --base=P -3 C" to 
>> generate
>> +patches for A, B and C, and the identifiers for P, X, Y, Z are appended
>> +at the end of the _first_ message.
>
>The contents of this look OK, but does it format correctly via
>AsciiDoc?  I suspect that only the first paragraph up to "of this
>shape:" would appear correctly and all the rest would become funny.

Sorry, just heard of AsciiDoc, I will try to use it to do the right format work.

>
>Also the definition of "base tree information" you need to have in
>the log message should be given somewhere in this documentation, not
>necessarily in the documentation of --base= option.
>
>Because the use of this new option is not an essential part of
>workflow of all users of format-patch, it may be a good idea to have
>its own separate section, perhaps between the "DISCUSSION" and
>"EXAMPLES" sections, titled "BASE TREE IDENTIFICATION", move the
>bulk of text above there with the specification of what "base tree
>info" consists of there.
>
>And shorten the description of the option to something like:
>
>--base=::
>   Record the base tree information to identify the state the
>   patch series applies to.  See the BASE TREE IDENTIFICATION
>section below for details.
>
>or something.

I'll restructure the descriptions in a resend.

>
>> diff --git a/builtin/log.c b/builtin/log.c
>> index 0d738d6..03cbab0 100644
>> --- a/builtin/log.c
>> +++ b/builtin/log.c
>> @@ -1185,6 +1185,82 @@ static int from_callback(const struct option *opt, 
>> const char *arg, int unset)
>>  return 0;
>>  }
>>  
>> +struct base_tree_info {
>> +

Re: [PATCH v3 00/16] port branch.c to use ref-filter's printing options

[Dennis Kaarsemaker]

On wo, 2016-03-30 at 15:09 +0530, Karthik Nayak wrote:
> 
> This is part of unification of the commands 'git tag -l, git branch -l
> and git for-each-ref'. This ports over branch.c to use ref-filter's
> printing options.
> 
> Initially posted here: $(gmane/279226). It was decided that this series
> would follow up after refactoring ref-filter parsing mechanism, which
> is now merged into master (9606218b32344c5c756f7c29349d3845ef60b80c).

Interaction between this series and something I've not yet been able to
identify seems to break t6302. In f08f64b (the merge commit that merges
this with pu), that test fails. But neither of its parents show the
same failure.

Full log of a failing 'make && make test':
https://ci.kaarsemaker.net/git/refs/heads/pu/718c0b31e51ab07181954fb5147e3283793553f4/artefact/test/log

Verbose output of the failing test:

expecting success: 
git for-each-ref --format="%(if)%(authorname)%(then)%(authorname): 
%(refname)%(end)" >actual &&
cat >expect <<-\EOF &&
A U Thor: refs/heads/master
A U Thor: refs/heads/side
A U Thor: refs/odd/spot

A U Thor: refs/tags/foo1.10
A U Thor: refs/tags/foo1.3
A U Thor: refs/tags/foo1.6
A U Thor: refs/tags/four
A U Thor: refs/tags/one

A U Thor: refs/tags/three
A U Thor: refs/tags/two
EOF
test_cmp expect actual

--- expect  2016-04-01 13:28:14.157855026 +
+++ actual  2016-04-01 13:28:14.153855026 +
@@ -2,6 +2,8 @@
 A U Thor: refs/heads/side
 A U Thor: refs/odd/spot
 
+
+
 A U Thor: refs/tags/foo1.10
 A U Thor: refs/tags/foo1.3
 A U Thor: refs/tags/foo1.6
not ok 34 - check %(if)...%(then)...%(end) atoms
#   
#   git for-each-ref 
--format="%(if)%(authorname)%(then)%(authorname): %(refname)%(end)" >actual &&
#   cat >expect <<-\EOF &&
#   A U Thor: refs/heads/master
#   A U Thor: refs/heads/side
#   A U Thor: refs/odd/spot
#   
#   A U Thor: refs/tags/foo1.10
#   A U Thor: refs/tags/foo1.3
#   A U Thor: refs/tags/foo1.6
#   A U Thor: refs/tags/four
#   A U Thor: refs/tags/one
#   
#   A U Thor: refs/tags/three
#   A U Thor: refs/tags/two
#   EOF
#   test_cmp expect actual
#   

expecting success: 
git for-each-ref 
--format="%(if)%(authorname)%(then)%(authorname)%(else)No author%(end): 
%(refname)" >actual &&
cat >expect <<-\EOF &&
A U Thor: refs/heads/master
A U Thor: refs/heads/side
A U Thor: refs/odd/spot
No author: refs/tags/double-tag
A U Thor: refs/tags/foo1.10
A U Thor: refs/tags/foo1.3
A U Thor: refs/tags/foo1.6
A U Thor: refs/tags/four
A U Thor: refs/tags/one
No author: refs/tags/signed-tag
A U Thor: refs/tags/three
A U Thor: refs/tags/two
EOF
test_cmp expect actual

--- expect  2016-04-01 13:28:14.161855026 +
+++ actual  2016-04-01 13:28:14.161855026 +
@@ -1,7 +1,9 @@
 A U Thor: refs/heads/master
 A U Thor: refs/heads/side
 A U Thor: refs/odd/spot
-No author: refs/tags/double-tag
+No author: refs/tags/annotated-tag
+No author: refs/tags/doubly-annotated-tag
+No author: refs/tags/doubly-signed-tag
 A U Thor: refs/tags/foo1.10
 A U Thor: refs/tags/foo1.3
 A U Thor: refs/tags/foo1.6
not ok 35 - check %(if)...%(then)...%(else)...%(end) atoms
#   
#   git for-each-ref 
--format="%(if)%(authorname)%(then)%(authorname)%(else)No author%(end): 
%(refname)" >actual &&
#   cat >expect <<-\EOF &&
#   A U Thor: refs/heads/master
#   A U Thor: refs/heads/side
#   A U Thor: refs/odd/spot
#   No author: refs/tags/double-tag
#   A U Thor: refs/tags/foo1.10
#   A U Thor: refs/tags/foo1.3
#   A U Thor: refs/tags/foo1.6
#   A U Thor: refs/tags/four
#   A U Thor: refs/tags/one
#   No author: refs/tags/signed-tag
#   A U Thor: refs/tags/three
#   A U Thor: refs/tags/two
#   EOF
#   test_cmp expect actual
#   

expecting success: 
git for-each-ref --format="%(refname:short): %(if)%(HEAD)%(then)Head 
ref%(else)Not Head ref%(end)" >actual &&
cat >expect <<-\EOF &&
master: Head ref
side: Not Head ref
odd/spot: Not Head ref
double-tag: Not Head ref
foo1.10: Not Head ref
foo1.3: Not Head ref
foo1.6: Not Head ref
four: Not Head ref
one: Not Head ref
signed-tag: Not Head ref
three: Not Head ref
two: Not Head ref
EOF
test_cmp expect actual

--- expect  2016-04-01 13:28:14.165855026 +
+++ actual  2016-04-01 13:28:14.165855026 +
@@ -1,7 +1,9 @@
 master: Head ref
 side: Not Head ref
 odd/spot: 

Re: GIT_CONFIG - what's the point?

[Jeff King]

On Thu, Mar 31, 2016 at 08:54:26PM -0400, Matthew Persico wrote:

> So, what's the point of GIT_CONFIG if only git-config uses it? Or did
> I miss a step?

There isn't a point to it. It's historical cruft that has been left in
to avoid breaking older scripts. The same thing is generally better
accomplished by using git-config's "--file" parameter. We should
probably do a better job of making that clear in the documentation.

Or possibly deprecate it and eventually remove it entirely, as discussed
in:

  
http://thread.gmane.org/gmane.linux.debian.devel.bugs.general/1195694/focus=257770

-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: What is an efficient way to get all blobs / trees that have notes attached?

[Johan Herland]

On Fri, Apr 1, 2016 at 12:51 PM, Sebastian Schuberth
 wrote:
> Hi,
>
> I'm curious whether there's a more efficient way to get a list of blobs /
> trees (and their names) that have notes attached than doing this:
>
> 1) Get all notes refs I'm interested in (git-for-each-ref).
>
> 2) For each notes ref, get the list of notes (git-notes list) and store them
> in a hash table that maps object hashes to notes.
>
> 3) Recursively list all blobs / trees (git-ls-tree) and look whether an
> object's hash is conatined in our table to get its notes.
>
> In particular 3) could be expensive for repos with a lot of files as we're
> looking at all of them just to see whether they have notes attached.

In (3), why would you need to search through _all_ blobs/trees? Would
it not be cheaper to simply query the object type of each annotated
object from (2)? I.e. something like:

for notes_ref in $(git for-each-ref refs/notes | cut -c 49-)
do
echo "--- $notes_ref ---"
for annotated_obj in $(git notes --ref=$notes_ref list | cut -c 41-)
do
type=$(git cat-file -t "$annotated_obj")
if test "$type" != "commit"
then
echo "$annotated_obj: $type"
fi
done
done

Can probably be made even faster by using the --batch option to cat-file...


...Johan

-- 
Johan Herland, 
www.herland.net
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Trouble with cat-file on tags

[Jeff King]

On Fri, Apr 01, 2016 at 12:00:44PM +0200, Sebastian Schuberth wrote:

> This means
> 
> $ git cat-file tag refs/tags/v0.1.2
> 
> displays the *contents* of the tag, not the tag itself.

Right. `cat-file` is about looking at object content.

> Which leads me to
> the next question: For a given name of an annotated tag, how to get the hash
> of the tag object? The solution I found for now:
> 
> $ git show-ref --tags -- v0.1.2
> 92b67e2b0626519ef8cd4e9cacb2bdafba6d53f0 refs/tags/v0.1.2

Here you just want to resolve the ref, without looking at the object.
The canonical way is:

  git rev-parse --verify v0.1.2

You can also use that to peel the tag to a commit, or a commit to a tree
(e.g., with "v0.1.2^{commit}").

-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: What is an efficient way to get all blobs / trees that have notes attached?

[Johan Herland]

On Fri, Apr 1, 2016 at 2:16 PM, Johan Herland  wrote:
> for notes_ref in $(git for-each-ref refs/notes | cut -c 49-)
> do
> echo "--- $notes_ref ---"
> for annotated_obj in $(git notes --ref=$notes_ref list | cut -c 41-)
> do
> type=$(git cat-file -t "$annotated_obj")
> if test "$type" != "commit"
> then
> echo "$annotated_obj: $type"
> fi
> done
> done
>
> Can probably be made even faster by using the --batch option to cat-file...

For example:

for notes_ref in $(git for-each-ref refs/notes | cut -c 49-)
do
echo "--- $notes_ref ---"
git notes --ref=$notes_ref list | cut -c 42- | git cat-file
--batch-check="%(objecttype) %(objectname)" | grep
'^\(\(blob\)\|\(tree\)\) '
done


...Johan

-- 
Johan Herland, 
www.herland.net
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: git alias quoting help

[shawn wilson]

On Fri, Apr 1, 2016 at 7:05 AM, Christian Couder
 wrote:
> On Wed, Mar 30, 2016 at 6:13 AM, shawn wilson  wrote:
>> I've also tried to make this a plain bash script (w/o the function or
>> if statements and am failing at the same place). The issue seems to be
>> with the quoting in the filter-branch | ls-files bit. Also, the end
>> goal here is to be able to move a directory from one repo and keep the
>> history.
>
> Did you try git subtree
> (https://github.com/git/git/blob/master/contrib/subtree/git-subtree.txt)?

Yeah, it looked like it left the repo in the same state as
subdirectory-filter and the merge failed. But I'll try it again when I
get to work.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Masking or hiding directories and files during a git submodule checkout

[Peter Waller]

Hi All,

My constraints are as follows:

* I use submodules to reference other people's code.
* I don't like to have to send commits to those authors of those 
submodules, they might not be accepted.

* I don't want to maintain my own fork of the submodule.
* Some submodules may contain lots of stuff I don't want to check out.
* I want other people (and existing tooling, such as `go get`) to be 
able to do an ordinary git clone --recursive and not get specific 
directories within some submodules checked not.


In my specific use case, I have:

/myfile
/submodule1/directory/nestedsubmodule2

That is, submodule1 itself uses submodules and contains a directory with 
nestedsubmodule2.


What I would like to achieve is to be able to specify in my repository 
configuration that I don't want anything living in 
/submodule1/directory/ to be checked out. But I want to check out 
everything else as normal.


Ideally, this would happen automatically during a git clone --recursive, 
probably by being specified somewhere such as .gitattributes. I envisage 
it this way so that it would work automatically with existing tooling.


* Is this possible now via any means?

* Is this against some fundamental philosophy?

* Is it conceivable that patches would be accepted to make this happen?

Thanks,

- Peter

For further information, this is a follow on from a mega-thread about 
vendoring packages in go [*]. I'm seeking a minimal solution where 
little additional tooling is required, and this is just one avenue I'm 
pursuing. One such solution (considered in this email) could be to avoid 
checking out specific directories which live in submodules. Of course if 
it requires changes to git, I'd have to wait for it to update before I 
can use that feature, but I'm OK with that, so long as we have a 
solution which works eventually.


[*] https://groups.google.com/d/topic/golang-nuts/AnMr9NL6dtc/discussion
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] imap-send.c: implements the GIT_CURL_DEBUG environment variable

[Torsten Bögershausen]

On 01.04.16 12:44, Elia Pinto wrote:
> Implements the GIT_CURL_DEBUG environment variable to allow a greater
> degree of detail of GIT_CURL_VERBOSE, in particular the complete
> transport header and all the data payload exchanged.
> It might be useful if a particular situation could require a more
> thorough debugging analysis.
> 
> Signed-off-by: Elia Pinto 
> ---
>  imap-send.c | 99 
> +++--
>  1 file changed, 97 insertions(+), 2 deletions(-)
> 
> diff --git a/imap-send.c b/imap-send.c
> index 4d3b773..cf79e7f 100644
> --- a/imap-send.c
> +++ b/imap-send.c
> @@ -1395,6 +1395,96 @@ static int append_msgs_to_imap(struct imap_server_conf 
> *server,
>  }
>  
>  #ifdef USE_CURL_FOR_IMAP_SEND
> +
> +static
> +void curl_dump(const char *text,
> +   FILE * stream, unsigned char *ptr, size_t size, char nohex)
Style: FILE *stream
> +{
> + size_t i;
> + size_t c;
c is typically a character.
'j' may be better, unless you find a longer,
and more descriptive name.

> +
> + unsigned int width = 0x10;
> +
> + if (nohex)
> + /* without the hex output, we can fit more on screen */
> + width = 0x40;
> +
> + fprintf(stream, "%s, %10.10ld bytes (0x%8.8lx)\n",
> + text, (long)size, (long)size);
> +
> + for (i = 0; i < size; i += width) {
> +
> + fprintf(stream, "%4.4lx: ", (long)i);
> +
> + if (!nohex) {
> + /* hex not disabled, show it */
> + for (c = 0; c < width; c++)
> + if (i + c < size)
> + fprintf(stream, "%02x ", ptr[i + c]);
> + else
> + fputs("   ", stream);
> + }
> +
> + for (c = 0; (c < width) && (i + c < size); c++) {
> + /* check for 0D0A; if found, skip past and start a new 
> line of output */
Should we write '\r' and '\n' instead for hex codes ?
and CRLF instead of 0D0A ? (Yes the RFC use hexcodes, but we write readable 
C-code)


--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: RPM spec file broken by README.md

[Christian Couder]

On Fri, Apr 1, 2016 at 1:23 PM, Christian Couder
 wrote:
> Hi!
>
> On Fri, Apr 1, 2016 at 7:35 AM, Ron Isaacson  wrote:
>> Hi everyone,
>>
>> I've noticed that "make rpm" is failing for 2.8.0 because README was
>> replaced with README.md. This line in git.spec is the culprit:
>>
>> %doc README COPYING Documentation/*.txt
>>
>> Would it be possible to change this to README.md to match the source
>> tree? The rpm packages build just fine with that change. Thank you
>> very much!
>
> Thanks for this bug report.
>
> Would you care to send a patch?

Ooops, it looks like Matthieu already sent a patch. Sorry for the noise.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: RPM spec file broken by README.md

[Christian Couder]

Hi!

On Fri, Apr 1, 2016 at 7:35 AM, Ron Isaacson  wrote:
> Hi everyone,
>
> I've noticed that "make rpm" is failing for 2.8.0 because README was
> replaced with README.md. This line in git.spec is the culprit:
>
> %doc README COPYING Documentation/*.txt
>
> Would it be possible to change this to README.md to match the source
> tree? The rpm packages build just fine with that change. Thank you
> very much!

Thanks for this bug report.

Would you care to send a patch?
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: GIT_CONFIG - what's the point?

[Christian Couder]

On Fri, Apr 1, 2016 at 2:54 AM, Matthew Persico
 wrote:
> Greetings.
>
> Given the GIT_CONFIG environment variable can change 'git config'
> behaves, it stands to reason that if GIT_CONFIG is defined, then ALL
> git commands obey the value of GIT_CONFIG and use that file for config
> info.
>
> As a test, exported GIT_CONFIG=/tmp/ohm, copied ~/.gitconfig to
> /tmp/ohm,

Is /tmp/ohm a directory? If that is the case, then you should probably
have exported "GIT_CONFIG=/tmp/ohm/.gitconfig", as the git config doc
says it specifies a filename.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: git alias quoting help

[Christian Couder]

On Wed, Mar 30, 2016 at 6:13 AM, shawn wilson  wrote:
> I've also tried to make this a plain bash script (w/o the function or
> if statements and am failing at the same place). The issue seems to be
> with the quoting in the filter-branch | ls-files bit. Also, the end
> goal here is to be able to move a directory from one repo and keep the
> history.

Did you try git subtree
(https://github.com/git/git/blob/master/contrib/subtree/git-subtree.txt)?
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

What is an efficient way to get all blobs / trees that have notes attached?

[Sebastian Schuberth]

Hi,

I'm curious whether there's a more efficient way to get a list of blobs 
/ trees (and their names) that have notes attached than doing this:


1) Get all notes refs I'm interested in (git-for-each-ref).

2) For each notes ref, get the list of notes (git-notes list) and store 
them in a hash table that maps object hashes to notes.


3) Recursively list all blobs / trees (git-ls-tree) and look whether an 
object's hash is conatined in our table to get its notes.


In particular 3) could be expensive for repos with a lot of files as 
we're looking at all of them just to see whether they have notes attached.


Regards,
Sebastian



--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 1/2] imap-send.c: implements the GIT_CURL_DEBUG environment variable

[Elia Pinto]

Implements the GIT_CURL_DEBUG environment variable to allow a greater
degree of detail of GIT_CURL_VERBOSE, in particular the complete
transport header and all the data payload exchanged.
It might be useful if a particular situation could require a more
thorough debugging analysis.

Signed-off-by: Elia Pinto 
---
 imap-send.c | 99 +++--
 1 file changed, 97 insertions(+), 2 deletions(-)

diff --git a/imap-send.c b/imap-send.c
index 4d3b773..cf79e7f 100644
--- a/imap-send.c
+++ b/imap-send.c
@@ -1395,6 +1395,96 @@ static int append_msgs_to_imap(struct imap_server_conf 
*server,
 }
 
 #ifdef USE_CURL_FOR_IMAP_SEND
+
+static
+void curl_dump(const char *text,
+ FILE * stream, unsigned char *ptr, size_t size, char nohex)
+{
+   size_t i;
+   size_t c;
+
+   unsigned int width = 0x10;
+
+   if (nohex)
+   /* without the hex output, we can fit more on screen */
+   width = 0x40;
+
+   fprintf(stream, "%s, %10.10ld bytes (0x%8.8lx)\n",
+   text, (long)size, (long)size);
+
+   for (i = 0; i < size; i += width) {
+
+   fprintf(stream, "%4.4lx: ", (long)i);
+
+   if (!nohex) {
+   /* hex not disabled, show it */
+   for (c = 0; c < width; c++)
+   if (i + c < size)
+   fprintf(stream, "%02x ", ptr[i + c]);
+   else
+   fputs("   ", stream);
+   }
+
+   for (c = 0; (c < width) && (i + c < size); c++) {
+   /* check for 0D0A; if found, skip past and start a new 
line of output */
+   if (nohex && (i + c + 1 < size) && ptr[i + c] == 0x0D
+   && ptr[i + c + 1] == 0x0A) {
+   i += (c + 2 - width);
+   break;
+   }
+   fprintf(stream, "%c",
+   (ptr[i + c] >= 0x20)
+   && (ptr[i + c] < 0x80) ? ptr[i + c] : '.');
+   /* check again for 0D0A, to avoid an extra \n if it's 
at width */
+   if (nohex && (i + c + 2 < size)
+   && ptr[i + c + 1] == 0x0D
+   && ptr[i + c + 2] == 0x0A) {
+   i += (c + 3 - width);
+   break;
+   }
+   }
+   fputc('\n', stream);/* newline */
+   }
+   fflush(stream);
+}
+
+static
+int curl_trace(CURL * handle, curl_infotype type,
+char *data, size_t size, void *userp)
+{
+   const char *text;
+   (void)handle;   /* prevent compiler warning */
+
+   switch (type) {
+   case CURLINFO_TEXT:
+   fprintf(stderr, "== Info: %s", data);
+   default:/* in case a new one is introduced to shock us 
*/
+   return 0;
+
+   case CURLINFO_HEADER_OUT:
+   text = "=> Send header";
+   break;
+   case CURLINFO_DATA_OUT:
+   text = "=> Send data";
+   break;
+   case CURLINFO_SSL_DATA_OUT:
+   text = "=> Send SSL data";
+   break;
+   case CURLINFO_HEADER_IN:
+   text = "<= Recv header";
+   break;
+   case CURLINFO_DATA_IN:
+   text = "<= Recv data";
+   break;
+   case CURLINFO_SSL_DATA_IN:
+   text = "<= Recv SSL data";
+   break;
+   }
+
+   curl_dump(text, stderr, (unsigned char *)data, size, 1);
+   return 0;
+}
+
 static CURL *setup_curl(struct imap_server_conf *srvc)
 {
CURL *curl;
@@ -1442,8 +1532,13 @@ static CURL *setup_curl(struct imap_server_conf *srvc)
 
curl_easy_setopt(curl, CURLOPT_UPLOAD, 1L);
 
-   if (0 < verbosity || getenv("GIT_CURL_VERBOSE"))
-   curl_easy_setopt(curl, CURLOPT_VERBOSE, 1L);
+   if (0 < verbosity )
+   if (getenv("GIT_CURL_DEBUG")) {
+   curl_easy_setopt(curl, CURLOPT_VERBOSE, 1);
+   curl_easy_setopt(curl, CURLOPT_DEBUGFUNCTION, 
curl_trace);
+   } else if (getenv("GIT_CURL_VERBOSE"))
+   curl_easy_setopt(curl, CURLOPT_VERBOSE, 1);
+
 
return curl;
 }
-- 
2.7.0.416.gbf6b42c.dirty

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4] git-send-pack: fix --all option when used with directory

[Stanislav Kolotinskiy]

On 31/03/16 23:28, Junio C Hamano wrote:
Thanks, will queue. 


Thanks a lot!

--
Regards,
Stanislav
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 2/2] http.c: implements the GIT_CURL_DEBUG environment variable

[Elia Pinto]

Implements the GIT_CURL_DEBUG environment variable to allow a greater
degree of detail of GIT_CURL_VERBOSE, in particular the complete
transport header and all the data payload exchanged.
It might be useful if a particular situation could require a more
thorough debugging analysis.

Signed-off-by: Elia Pinto 
---
 http.c | 97 +-
 1 file changed, 96 insertions(+), 1 deletion(-)

diff --git a/http.c b/http.c
index dfc53c1..079779d 100644
--- a/http.c
+++ b/http.c
@@ -437,6 +437,97 @@ static void set_curl_keepalive(CURL *c)
 }
 #endif
 
+
+static
+void curl_dump(const char *text,
+ FILE * stream, unsigned char *ptr, size_t size, char nohex)
+{
+   size_t i;
+   size_t c;
+
+   unsigned int width = 0x10;
+
+   if (nohex)
+   /* without the hex output, we can fit more on screen */
+   width = 0x40;
+
+   fprintf(stream, "%s, %10.10ld bytes (0x%8.8lx)\n",
+   text, (long)size, (long)size);
+
+   for (i = 0; i < size; i += width) {
+
+   fprintf(stream, "%4.4lx: ", (long)i);
+
+   if (!nohex) {
+   /* hex not disabled, show it */
+   for (c = 0; c < width; c++)
+   if (i + c < size)
+   fprintf(stream, "%02x ", ptr[i + c]);
+   else
+   fputs("   ", stream);
+   }
+
+   for (c = 0; (c < width) && (i + c < size); c++) {
+   /* check for 0D0A; if found, skip past and start a new 
line of output */
+   if (nohex && (i + c + 1 < size) && ptr[i + c] == 0x0D
+   && ptr[i + c + 1] == 0x0A) {
+   i += (c + 2 - width);
+   break;
+   }
+   fprintf(stream, "%c",
+   (ptr[i + c] >= 0x20)
+   && (ptr[i + c] < 0x80) ? ptr[i + c] : '.');
+   /* check again for 0D0A, to avoid an extra \n if it's 
at width */
+   if (nohex && (i + c + 2 < size)
+   && ptr[i + c + 1] == 0x0D
+   && ptr[i + c + 2] == 0x0A) {
+   i += (c + 3 - width);
+   break;
+   }
+   }
+   fputc('\n', stream);/* newline */
+   }
+   fflush(stream);
+}
+
+static
+int curl_trace(CURL * handle, curl_infotype type,
+char *data, size_t size, void *userp)
+{
+   const char *text;
+   (void)handle;   /* prevent compiler warning */
+
+   switch (type) {
+   case CURLINFO_TEXT:
+   fprintf(stderr, "== Info: %s", data);
+   default:/* in case a new one is introduced to shock us 
*/
+   return 0;
+
+   case CURLINFO_HEADER_OUT:
+   text = "=> Send header";
+   break;
+   case CURLINFO_DATA_OUT:
+   text = "=> Send data";
+   break;
+   case CURLINFO_SSL_DATA_OUT:
+   text = "=> Send SSL data";
+   break;
+   case CURLINFO_HEADER_IN:
+   text = "<= Recv header";
+   break;
+   case CURLINFO_DATA_IN:
+   text = "<= Recv data";
+   break;
+   case CURLINFO_SSL_DATA_IN:
+   text = "<= Recv SSL data";
+   break;
+   }
+
+   curl_dump(text, stderr, (unsigned char *)data, size, 1);
+   return 0;
+}
+
+
 static CURL *get_curl_handle(void)
 {
CURL *result = curl_easy_init();
@@ -532,7 +623,11 @@ static CURL *get_curl_handle(void)
"your curl version is too old (>= 7.19.4)");
 #endif
 
-   if (getenv("GIT_CURL_VERBOSE"))
+   if (getenv("GIT_CURL_DEBUG")) {
+   curl_easy_setopt(result, CURLOPT_VERBOSE, 1);
+   curl_easy_setopt(result, CURLOPT_DEBUGFUNCTION, curl_trace);
+   curl_easy_setopt(result, CURLOPT_DEBUGDATA, NULL);
+   } else if (getenv("GIT_CURL_VERBOSE"))
curl_easy_setopt(result, CURLOPT_VERBOSE, 1);
 
curl_easy_setopt(result, CURLOPT_USERAGENT,
-- 
2.7.0.416.gbf6b42c.dirty

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] git-send-pack: Fix --all option when used with directory

[Stanislav Kolotinskiy]

Thanks a lot, Jeff, your explanation really helped!

--
Regards,
Stanislav

On 24/03/16 20:02, Jeff King wrote:

On Thu, Mar 24, 2016 at 07:47:12PM +0200, Stanislav Kolotinskiy wrote:


Thanks for noticing; the above explanation however does not make it
very clear why the symptom exhibits itself only when "directory" is
given (it also is unclear if "target" being a directory is special,
or if any remote repository specification, e.g. host:/path/to/dir,
triggers the same problem).

I'll think about a better explanation and will post it here - thanks.

Feel free to steal from the explanation I posted earlier.

-Peff

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: git alias quoting help

[shawn wilson]

I think I finally figured out how I want to do this:

git remote add temp ..//
git fetch temp
git merge -s ours --no-commit temp/master
git read-tree --prefix= -u temp/master:
git commit -m "foo"

However, when I do this, I've got all of the commits from the original
(temp) repo. How do I prune these out? None of the files show up, but
I do see reference to them in: git log --stat. And nothing I do with
gc or prune seem to have any affect.

On Fri, Apr 1, 2016 at 2:10 AM, shawn wilson  wrote:
> FWIW, I (finally) found two projects that like they'll do what I want:
> git-splits and git_filter
> The later was lacking in documentation and after the build I couldn't
> figure it out at a glance and I think git-splits will DWIW.
>
> On Thu, Mar 31, 2016 at 10:27 AM, shawn wilson  wrote:
>> BTW, just trying to get filter-branch to interpret the bash script
>> string correctly now and it still isn't working:
>>
>> git filter-branch -f --prune-empty --index-filter "\
>>   git ls-files -s | \
>>   sed \"s-\\t\\\"*-&${1}-\" | \
>>   GIT_INDEX_FILE=\$GIT_INDEX_FILE.new \
>>   git update-index --index-info && \
>> mv \$GIT_INDEX_FILE.new \$GIT_INDEX_FILE \
>> " HEAD
>>
>>  I'm guessing bash is grabbing my actual bash shell is grabbing the
>> GIT_INDEX_FILE declaration for itself. If this is the case, I'm not
>> sure how to stop it - tried var\=\$var.new and that passes the '\='
>> which totally messes things up.
>>
>> Rewrite ef54b77e59c7f4e18f00168ba88a8d2fee795802 (1/76)mv: cannot stat
>> `//.git-rewrite/t/../index.new': No such file or directory
>> index filter failed:   git ls-files -s |   sed
>> "s-\t\"*-/adjoin/-" |   GIT_INDEX_FILE=$GIT_INDEX_FILE.new
>> git update-index --index-info && mv $GIT_INDEX_FILE.new
>> $GIT_INDEX_FILE
>>
>> On Wed, Mar 30, 2016 at 12:13 AM, shawn wilson  wrote:
>>> I've also tried to make this a plain bash script (w/o the function or
>>> if statements and am failing at the same place). The issue seems to be
>>> with the quoting in the filter-branch | ls-files bit. Also, the end
>>> goal here is to be able to move a directory from one repo and keep the
>>> history. While this works if I do it at the command line, it's just
>>> too many steps (is tedious). Also, if there's a way to do the same
>>> thing with multiple directories in one shot, (or make this work with
>>> something like: cookbooks/{a,b,c} # as a parameter) that'd be perfect.
>>>
>>>   reapdir = "!f() { \
>>> if [ -d "$1" ] ; then \
>>>   git filter-branch --prune-empty --subdirectory-filter "$1" -- --all 
>>> && \
>>>   git gc --aggressive && \
>>>   git prune && \
>>>   git filter-branch -f --prune-empty --index-filter '\
>>> git ls-files -s \
>>>   | sed \"s-\\t-&$1-\" \
>>>   | GIT_INDEX_FILE=$GIT_INDEX_FILE.new git update-index
>>> --index-info && \
>>> mv $GIT_INDEX_FILE.new $GIT_INDEX_FILE'; \
>>> else \
>>>   echo "No directory $1"; \
>>> fi; }; \
>>>   f"
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 5/5] t/t5520: test --[no-]autostash with pull.rebase=true

[Mehul Jain]

Hi Eric,

On Thu, Mar 31, 2016 at 2:01 AM, Eric Sunshine  wrote:
> One other possibility would be to make this all table-driven by
> collecting all of the above state information into a table and then
> feeding that into a function (either as its argument list or via
> stdin). For instance:
>
> test_autostash <<\-EOF
> ok,--rebase,rebase.autostash=true
> ok,--rebase --autostash,rebase.autostash=true
> ok,--rebase --autostash,rebase.autostash=false
> ok,--rebase --autostash,rebase.autostash=
> err,--rebase --no-autostash,rebase.autostash=true
> err,--rebase --no-autostash,rebase.autostash=false
> err,--rebase --no-autostash,rebase.autostash=
> ok,--autostash,pull.rebase=true
> err,--no-autostash,pull.rebase=true
>EOF
>
> The function would loop over the input, split each line apart by
> setting IFS=, and then run the test based upon the state information.
> "ok" means autostash is expected to succeed, and err means it is
> expected to fail. The function would want to specially recognize the
> "foo.bar=" in the last argument in order to invoke test_unconfig()
> rather than test_config().

I tried out this method also. Below is the script that I wrote for this:

---

test_autostash () {
OLDIFS=$IFS
IFS=',='
while read -r expect cmd config_variable value
do
test_expect_success "$cmd, $config_variable=$value" '
if [ "$value" = "" ]; then
test_unconfig $config_variable
else
test_config $config_variable $value
fi &&

git reset --hard before-rebase &&
echo dirty >new_file &&
git add new_file &&

if [ $expect = "ok" ]; then
git pull '$cmd' . copy &&
echo test_cmp_rev HEAD^ copy &&
test "$(cat new_file)" = dirty &&
test "$(cat file)" = "modified again"
else
test_must_fail git pull '$cmd' . copy 2>err &&
test_i18ngrep "uncommitted changes." err
fi
'
done
IFS=$OLDIFS
}


test_autostash <<-\EOF
ok,--rebase,rebase.autostash=true
ok,--rebase --autostash,rebase.autostash=true
ok,--rebase --autostash,rebase.autostash=false
ok,--rebase --autostash,rebase.autostash=
err,--rebase --no-autostash,rebase.autostash=true
err,--rebase --no-autostash,rebase.autostash=false
err,--rebase --no-autostash,rebase.autostash=
ok,--autostash,pull.rebase=true
err,--no-autostash,pull.rebase=true
EOF


---

Things worked out perfectly.

Unfortunately there was a strange behaviour that I noticed
and frankly I don't understand why it happened.

In test_autostash() there's a line

echo test_cmp_rev HEAD^ copy &&

Originally it should have been

test_cmp_rev HEAD^ copy &&

but this raise following error while testing

./t5520-pull.sh: 684: eval: diff -u: not found

I'm not able to understand why putting an "echo" before
test_cmp didn't raise the above error. This looks quite
strange. Any thoughts?

Though the above code works perfectly and can be used in
place of previous tests. Only problem remains is tests titles.
Currently with this script, test titles will be:

ok 21 - --rebase, rebase.autostash=true
ok 22 - --rebase --autostash, rebase.autostash=true
ok 23 - --rebase --autostash, rebase.autostash=false
ok 24 - --rebase --autostash, rebase.autostash=
ok 25 - --rebase --no-autostash, rebase.autostash=true
ok 26 - --rebase --no-autostash, rebase.autostash=false
ok 27 - --rebase --no-autostash, rebase.autostash=
ok 28 - --autostash, pull.rebase=true
ok 29 - --no-autostash, pull.rebase=true

Any thoughts/suggestions on them?

Thanks,
Mehul
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] doc: clarify that notes can be attached to any type of stored object

[Sebastian Schuberth]

Signed-off-by: Sebastian Schuberth 
---
 Documentation/git-notes.txt | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/Documentation/git-notes.txt b/Documentation/git-notes.txt
index 8de3499..5375d98 100644
--- a/Documentation/git-notes.txt
+++ b/Documentation/git-notes.txt
@@ -234,8 +234,9 @@ which operation triggered the update, and the commit 
authorship is
 determined according to the usual rules (see linkgit:git-commit[1]).
 These details may change in the future.
 
-It is also permitted for a notes ref to point directly to a tree
-object, in which case the history of the notes can be read with
+It is also permitted for a notes ref to point to any other object in
+the object store besides commit objects, that is annotated tags, blobs
+or trees. For the latter, the history of the notes can be read with
 `git log -p -g `.
 
 
-- 
2.8.0.windows.1


--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Trouble with cat-file on tags

[Sebastian Schuberth]

On 4/1/2016 11:32, Sebastian Schuberth wrote:


However, I still get information about the commit oject iintsead of the
tag object. Is this expected?


Solved this one, too: Yes it is. I was misreading the docs:

"If  is specified, the raw (though uncompressed) contents of the 
 will be returned."


This means

$ git cat-file tag refs/tags/v0.1.2

displays the *contents* of the tag, not the tag itself. Which leads me 
to the next question: For a given name of an annotated tag, how to get 
the hash of the tag object? The solution I found for now:


$ git show-ref --tags -- v0.1.2
92b67e2b0626519ef8cd4e9cacb2bdafba6d53f0 refs/tags/v0.1.2

Regards,
Sebastian



--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Trouble with cat-file on tags

[Sebastian Schuberth]

On 4/1/2016 11:26, Sebastian Schuberth wrote:


---8<---
$ git tag test-tag

$ git tag -l
test-tag
v0.0.3
v0.0.4
v0.1.0
v0.1.1
v0.1.2

$ git cat-file tag refs/tags/test-tag
fatal: git cat-file refs/tags/test-tag: bad file
---8<---


Alright, I just found out why that is: Lighweight tags are not stored as 
Git objects. As soon as I make it an annoted tag by specifying a 
message, "cat-file tag" do esnot display a fatal error anymore.


However, I still get information about the commit oject iintsead of the 
tag object. Is this expected?


Regards,
Sebastian


--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Trouble with cat-file on tags

[Sebastian Schuberth]

Hi,

I was trying to use cat-file to get the hash of a tag object (not the hash of 
the commit object the tag points to), and I'm running into some issues. At the 
example of a cloned gerry [1] repository:

---8<---
$ git tag test-tag

$ git tag -l
test-tag
v0.0.3
v0.0.4
v0.1.0
v0.1.1
v0.1.2

$ git cat-file tag refs/tags/test-tag
fatal: git cat-file refs/tags/test-tag: bad file
---8<---

So for a newly created local tag, cat-file does not seem to work. However:

---8<---
$ git cat-file tag refs/tags/v0.1.2
object 91b0d21eba039e5ba0a90104c9c485735576dcbf
type commit
tag v0.1.2
tagger Travis Truman  1452693317 -0500

Version 0.1.2
---8<---

For an existing tag, git-file suddenly *does* seem to work, although I'm 
puzzled why I'm getting info on the commit object here. I thought "cat-file 
tag" should explicitly make "cat-file" list information about the tag object 
itself, not about the commit object the tag points to.

Thoughts?

[1] https://github.com/trumant/gerry

Regards,
Sebastian



--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv2 3/4] bundle: don't leak an fd in case of early return

[Philip Oakley]

From: "Stefan Beller" 
On Thu, Mar 31, 2016 at 12:00 PM, Philip Oakley  
wrote:

From: "Stefan Beller" 


In successful operation `write_pack_data` will close the `bundle_fd`,
but when we exit early, we need to take care of the file descriptor
as well as the lock file ourselves. The lock file may be deleted at the
end of running the program, but we are in library code, so we should
not rely on that.

Helped-by: Jeff King 
Signed-off-by: Stefan Beller 



Has this been tested on Windows? I had a similar problem very recently
https://groups.google.com/forum/#!msg/git-for-windows/6LPxf9xZKhI/-s7XD18yCwAJ
where a bad rev-list-arg would cause the `bundle create` to die, and, on
windows, leave the incomplete bundle file locked.

dscho suggested one possible solution for that, but I haven't had any 
time

to try any patches.


I think with Jeffs suggestion to only rollback the lock in case of
(!bundle_to_stdout)
we're not making it worse. I do not have a Windows machine at hand to test 
it :(


Thant's fine. I just wanted to make folks aware of the problem so it doesn't 
get worse.


It looks like the cause is that the 
bundle.c:compute_and_write_prerequisites(), which includes parsing the 
arg-list (and die on bad arg), is called after the creation and locking of 
the bundle file descriptor and header, thus leaving it hanging on Windows.


--

Philip 


--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] git.spec: use README.md, not README

[Matthieu Moy]

The file was renamed in 4ad21f5 (README: use markdown syntax,
2016-02-25), but that commit forgot to update git.spec.in.

Reported-by: Ron Isaacson 
Signed-off-by: Matthieu Moy 
---
 git.spec.in | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/git.spec.in b/git.spec.in
index d61d537..bfd1cfb 100644
--- a/git.spec.in
+++ b/git.spec.in
@@ -146,7 +146,7 @@ rm -rf $RPM_BUILD_ROOT
 %files -f bin-man-doc-files
 %defattr(-,root,root)
 %{_datadir}/git-core/
-%doc README COPYING Documentation/*.txt
+%doc README.md COPYING Documentation/*.txt
 %{!?_without_docs: %doc Documentation/*.html Documentation/howto}
 %{!?_without_docs: %doc Documentation/technical}
 %{_sysconfdir}/bash_completion.d
-- 
2.7.2.334.g35ed2ae.dirty

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] Feature: custom guitool commands can now have custom keyboard shortcuts

[harish k]

Hi David,

Actually Im a TCL primer.  This is the first time Im dealing with.
That is why I kept it simple ( ie both accel-key and accel-label need
to be defined in config ).

I think, git-cola is using Qt style representation accel-key in the config file.

Is git-cola is an official tool from git ? like git-gui?
if not ,
My suggesion is, it is better to seperate config fields according to
application-domain
like, "git-gui-accel = " etc..
Other vise there is good chance for conflicts. ( Eg: consider the case
 that,  was assined to a custom tool by git-cola )

Currently this patch will not handle any conflicting shortcuts. I
think custom shortcuts will overwrite the other.


On Thu, Mar 31, 2016 at 10:11 PM, David Aguilar  wrote:
> Hello,
>
> On Tue, Mar 29, 2016 at 11:38:10AM +, Harish K wrote:
>> ---
>>  git-gui/lib/tools.tcl | 16 +---
>>  1 file changed, 13 insertions(+), 3 deletions(-)
>>
>> diff --git a/git-gui/lib/tools.tcl b/git-gui/lib/tools.tcl
>> index 6ec9411..749bc67 100644
>> --- a/git-gui/lib/tools.tcl
>> +++ b/git-gui/lib/tools.tcl
>> @@ -38,7 +38,7 @@ proc tools_create_item {parent args} {
>>  }
>>
>>  proc tools_populate_one {fullname} {
>> - global tools_menubar tools_menutbl tools_id
>> + global tools_menubar tools_menutbl tools_id repo_config
>>
>>   if {![info exists tools_id]} {
>>   set tools_id 0
>> @@ -61,9 +61,19 @@ proc tools_populate_one {fullname} {
>>   }
>>   }
>>
>> - tools_create_item $parent command \
>> + if {[info exists repo_config(guitool.$fullname.accelerator)] && [info 
>> exists repo_config(guitool.$fullname.accelerator-label)]} {
>> + set accele_key $repo_config(guitool.$fullname.accelerator)
>> + set accel_label 
>> $repo_config(guitool.$fullname.accelerator-label)
>> + tools_create_item $parent command \
>>   -label [lindex $names end] \
>> - -command [list tools_exec $fullname]
>> + -command [list tools_exec $fullname] \
>> + -accelerator $accel_label
>> + bind . $accele_key [list tools_exec $fullname]
>> + } else {
>> + tools_create_item $parent command \
>> + -label [lindex $names end] \
>> + -command [list tools_exec $fullname]
>> + }
>>  }
>>
>>  proc tools_exec {fullname} {
>>
>> --
>> https://github.com/git/git/pull/220
>
> We also support "custom guitools" in git-cola using this same
> mechanism.  If this gets accepted then we'll want to make
> similar change there.
>
> There's always a small risk that user-defined tools can conflict
> with builtin shortcuts, but otherwise this seems like a pretty
> nice feature.  Curious, what is the behavior in the event of a
> conflict?  Do the builtins win?  IIRC, Qt handles this by
> disabling the shortcut and warning that it's ambiguous.
>
> Please documentation guitool..accellerator[-label] in
> Documentation/config.txt otherwise users will not know that it
> exists.
>
> It would also be good for the docs to clarify what the
> accelerators look like in case we need to munge them when making
> it work in cola via Qt, which has its own mechanism for
> associating actions with shortcuts.  Documented examples with
> one and two modifier keys would be helpful.
>
>
> cheers,
> --
> David



-- 

-Regards
Harish.K
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: git alias quoting help

[shawn wilson]

FWIW, I (finally) found two projects that like they'll do what I want:
git-splits and git_filter
The later was lacking in documentation and after the build I couldn't
figure it out at a glance and I think git-splits will DWIW.

On Thu, Mar 31, 2016 at 10:27 AM, shawn wilson  wrote:
> BTW, just trying to get filter-branch to interpret the bash script
> string correctly now and it still isn't working:
>
> git filter-branch -f --prune-empty --index-filter "\
>   git ls-files -s | \
>   sed \"s-\\t\\\"*-&${1}-\" | \
>   GIT_INDEX_FILE=\$GIT_INDEX_FILE.new \
>   git update-index --index-info && \
> mv \$GIT_INDEX_FILE.new \$GIT_INDEX_FILE \
> " HEAD
>
>  I'm guessing bash is grabbing my actual bash shell is grabbing the
> GIT_INDEX_FILE declaration for itself. If this is the case, I'm not
> sure how to stop it - tried var\=\$var.new and that passes the '\='
> which totally messes things up.
>
> Rewrite ef54b77e59c7f4e18f00168ba88a8d2fee795802 (1/76)mv: cannot stat
> `//.git-rewrite/t/../index.new': No such file or directory
> index filter failed:   git ls-files -s |   sed
> "s-\t\"*-/adjoin/-" |   GIT_INDEX_FILE=$GIT_INDEX_FILE.new
> git update-index --index-info && mv $GIT_INDEX_FILE.new
> $GIT_INDEX_FILE
>
> On Wed, Mar 30, 2016 at 12:13 AM, shawn wilson  wrote:
>> I've also tried to make this a plain bash script (w/o the function or
>> if statements and am failing at the same place). The issue seems to be
>> with the quoting in the filter-branch | ls-files bit. Also, the end
>> goal here is to be able to move a directory from one repo and keep the
>> history. While this works if I do it at the command line, it's just
>> too many steps (is tedious). Also, if there's a way to do the same
>> thing with multiple directories in one shot, (or make this work with
>> something like: cookbooks/{a,b,c} # as a parameter) that'd be perfect.
>>
>>   reapdir = "!f() { \
>> if [ -d "$1" ] ; then \
>>   git filter-branch --prune-empty --subdirectory-filter "$1" -- --all && 
>> \
>>   git gc --aggressive && \
>>   git prune && \
>>   git filter-branch -f --prune-empty --index-filter '\
>> git ls-files -s \
>>   | sed \"s-\\t-&$1-\" \
>>   | GIT_INDEX_FILE=$GIT_INDEX_FILE.new git update-index
>> --index-info && \
>> mv $GIT_INDEX_FILE.new $GIT_INDEX_FILE'; \
>> else \
>>   echo "No directory $1"; \
>> fi; }; \
>>   f"
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html