Re: [PATCH 2/2] SubmittingPatches: hint at gitk's "Copy commit summary" command

[Jacob Keller]

On Fri, Aug 26, 2016 at 11:27 AM, Junio C Hamano  wrote:
> Beat Bolli  writes:
>
>> @@ -124,7 +124,8 @@ archive, summarize the relevant points of the discussion.
>>  If you want to reference a previous commit in the history of a stable
>>  branch use the format "abbreviated sha1 (subject, date)". So for example
>>  like this: "Commit f86a374 (pack-bitmap.c: fix a memleak, 2015-03-30)
>> -noticed [...]".
>> +noticed [...]". The "Copy commit summary" command of gitk generates this
>> +format.
>
> (continuing from my 1/2 review)  And if people agree that the format
> gitk already uses is better, this text should probably read more
> like:
>
> If you want to reference a previous commit in the history of a
> stable branch, use the format "abbreviated sha1 (subject, date)",
> with the subject enclosed in a pair of double-quotes, like this:
>
> Commit f86a374 ("pack-bitmap.c: fix a memleak", 2015-03-30)
> noticed that ...
>
> The "Copy commit summary" command of gitk can be used to obtain this
> format.


Tangent, but I was wondering if this would make a good built-in
format, but then I looked and realized that the built-in formats
didn't make much sense to me... I'm not sure where the actual format
gets displayed so I am wondering if we had any thoughts about adding
some --pretty=summary that we could add? I know it can be implemented
via a user defined format but thought it might be worth building it in
since it's a pretty common use case?

Thanks,
Jake
--
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

stable as subset of develop

[Brett Porter]

In a small group, develop is the branch where all fixes/additions/... from topic
branches are merged rather dynamically. Thorough testing of commits may lag 
behind,
but when we think one is a pretty good commit we want to identify it as (at 
least
relatively) the latest stable. We could tag it, but we would like these stable 
commits to
be a branch in the sense that each commit points back to a previous commit.

Merging from a development branch commit to stable isn't quite what we want. It 
seems
more like:

  checkout the new good development commit
  change HEAD to the head of the stable branch
  git add --all
  git commit
  (maybe tag the new commit with the hash of the chosen development commit)

Will that work (one thing beyond my current understanding is if there are index 
complications)?
Other ideas?

This could help with applying successively more intense testing over time and 
chase down
where problems arose.
--
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: [RFC] Proposed questions for "Git User's Survey 2016"

[Jakub Narębski]

[for some reason this email is not yet present on GMane NNTP interface]

On 26 August 2016 at 21:12, David Bainbridge  wrote:
> Hi Jakub,
>
> This is excellent news!
>
> As when you last performed the survey it would be useful for us to be able
> to see what the users in our organization (Ericsson) think of Git, and there
> are many more than when the survey was last performed.
>
> Do any other organizations of any type have this need?

Yes, I will offer separate channels (that can be separately analyzed,
and that you can get anonymous data for specific channel) for every
organization and/or company that want's it (within reason)... this time
with names anonymized from start (unless requested not to).

> As for the questions, it might be useful to have a question related to repo
> management systems being used.
> Gerrit
> Kallithea
> Rhodecode
> Atlassian Bitbucket
> GitHub Enterprise
> GitHub.com
> GitLab
> Deveo
> etc.
>
> This is not directly about Git of course but seeing the extent to which
> these are used, and the proportion of users using them might be useful.

This is somewhat present in current version of survey, namely
there is question about *types* of tools (with git repository management,
git hosting tools and code review tools included), and there is free-form
question asking to enumerate tools one uses.  That is, as above, but
more generic, and not multiple choice.

I will think about adding such question.

Regards
-- 
Jakub Narębski
--
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 v12 12/13] apply: learn to use a different index file

[Christian Couder]

On Thu, Aug 11, 2016 at 10:17 PM, Junio C Hamano  wrote:
> Christian Couder  writes:
>
>> Sometimes we want to apply in a different index file.
>>
>> Before the apply functionality was libified it was possible to
>> use the GIT_INDEX_FILE environment variable, for this purpose.
>>
>> But now, as the apply functionality has been libified, it should
>> be possible to do that in a libified way.
>>
>> Signed-off-by: Christian Couder 
>> ---
>>  apply.c | 10 --
>>  apply.h |  1 +
>>  2 files changed, 9 insertions(+), 2 deletions(-)
>>
>> diff --git a/apply.c b/apply.c
>> index 2ec2a8a..7e561a4 100644
>> --- a/apply.c
>> +++ b/apply.c
>> @@ -4674,8 +4674,14 @@ static int apply_patch(struct apply_state *state,
>>   state->apply = 0;
>>
>>   state->update_index = state->check_index && state->apply;
>> - if (state->update_index && state->newfd < 0)
>> - state->newfd = hold_locked_index(state->lock_file, 1);
>> + if (state->update_index && state->newfd < 0) {
>> + if (state->index_file)
>> + state->newfd = 
>> hold_lock_file_for_update(state->lock_file,
>> +  
>> state->index_file,
>> +  
>> LOCK_DIE_ON_ERROR);
>> + else
>> + state->newfd = hold_locked_index(state->lock_file, 1);
>> + }
>>
>>   if (state->check_index && read_cache() < 0) {
>>   error(_("unable to read index file"));
>
> Here is a call to read_cache() that reads the default index file on
> the filesystem into the default in-core index "the_index".
>
> Shouldn't it be reading from state->index_file instead?

Yes, it should.

> If we limit the review only to the context of your series, I think
>
> fall_back_threeway()
>  -> build_fake_ancestor() -- uses index_path to use custom index
>  -> discard_cache()
>  -> read_cache_from(index_path) -- reads back the fake ancestor
>  -> write_index_as_tree() -- writes the fake_ancestor
>  -> run_apply(index_path)
> -> apply_all_patches()
>-> apply_patch()
>
> is the only codepath that uses a custom index file, so when the
> control reaches this function with a custom index file, the in-core
> index is already populated, making read_cache() a no-op, and that is
> the only thing that makes the resulting code avoid triggering this
> bug, but as part of a general "libified" codepath,

Yeah, I agree with this reasoning.

> I think it should
> be made to read from state->index_file using read_cache_from().

Yeah, I will change it.

> I only noticed this call to read_cache(), but there may be others
> lurking nearby.

Yeah, there is another one in get_current_sha1() which is only called
by build_fake_ancestor() (from apply.c not from builtin/am.c as there
is a function with this name in both files), but this function is
currently called only when --build-fake-ancestor is passed which is
not the case in run_apply() in am.c. I will change it too.

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 v14 22/27] bisect--helper: `bisect_replay` shell function in C

[Junio C Hamano]

Pranit Bauva  writes:

> +static int get_next_word(struct strbuf *line, struct strbuf *word)
> +{
> + int i;
> + for (i = 0; line->buf[i] != ' ' && line->buf[i] != '\0'; i++)
> + strbuf_addch(word, line->buf[i]);
> +
> + return 0;
> +}

This looks like a very non-standard way to use a string buffer.  The
function does not even have to take line as a whole strbuf, because
the logic does not even look at line->len and relies on line->buf[]
to be NUL terminated, which means the first parameter should instead
be "const char *buf", and the caller would feed then line.buf to the
function.

And I highly suspect that this is a very suboptimal interface, if
the caller then has to compute up to which byte in the line.buf to
splice away to get to the "next word" again.

A better alternative with higher level of abstraction would instead
be to keep the two strbuf parameters as-is, but have this function
responsible for "moving" the first word of "line" strbuf into "word"
strbuf.  Then the caller can repeatedly call this function to get
each word in "word" strbuf until "line" becomes empty if it wants to
implement "a word at a time" operation.

If that higher level of abstraction is too limiting for the caller
(and also that would be just as inefficient as the patch under
discussion), another alternative would be to have the caller
maintain "the current byte position in the input" and
do something like:

int pos = 0;

while (pos < line.len) {
strbuf_reset();
get_next_word(line.buf, pos, word);
pos += word.len;
do a thing on "word";
}

to implement "a word at a time" operation.  For this to work,
get_next_word() would need to skip the leading blanks itself, of
course.

In any case, I won't comment on the body of the function too deeply;
it will probably become a lot cleaner if you employed the "retval +
goto finish:" pattern for error handling.  Use of dequote seems
correct from the cursory look, too.

--
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 v11 0/8] submodule inline diff format

[Jacob Keller]

From: Jacob Keller 

> On Fri, Aug 26, 2016 at 1:04 PM, Jeff King  wrote:
> > On Fri, Aug 26, 2016 at 07:58:07PM +, Keller, Jacob E wrote:
> >
> >> > >  char *git_pathdup_submodule(const char *path, const char *fmt,
> >> > > ...)
> >> > >  {
> >> > > +   int err;
> >> > > va_list args;
> >> > > struct strbuf buf = STRBUF_INIT;
> >> > > va_start(args, fmt);
> >> > > -   do_submodule_path(, path, fmt, args);
> >> > > +   err = do_submodule_path(, path, fmt, args);
> >> > > va_end(args);
> >> > > +   if (err)
> >> >
> >> > Here we need a strbuf_release() to avoid a memory leak?
> >>
> >> No, cause we "strbuf_detach" after this to return the buffer? Or is
> >> that not safe?
> >
> > That code path is OK. I think the question is whether you need to
> > release the buffer in the "err" case where you return NULL and don't hit
> > the strbuf_detach.
> >
> > IOW, does do_submodule_path() promise that when it returns an error,
> > "buf" has been left uninitialized? Some of our strbuf functions do, but
> > I do not know offhand about do_submodule_path().
> >
> > -Peff
> 
> We probably should release for the error case. I'll do that. I don't
> believe do_submodule_path ensures that the passed in argument is
> guaranteed to not be initialized or used.
> 
> Thanks,
> Jake

Here's the squash for this fix.

Thanks,
Jake

-->8

>From Jacob Keller 
>From 9cf89634e6f2b0f3f90f43a553f55eb57bb2f662 Mon Sep 17 00:00:00 2001
From: Jacob Keller 
Date: Fri, 26 Aug 2016 16:06:54 -0700
Subject: [PATCH] squash! allow do_submodule_path to work even if submodule
 isn't checked out

Add a missing strbuf_release() when returning during the error flow of
git_pathdup_submodule().

Signed-off-by: Jacob Keller 
---
 path.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/path.c b/path.c
index e9369f75319d..b8515973252c 100644
--- a/path.c
+++ b/path.c
@@ -525,8 +525,10 @@ char *git_pathdup_submodule(const char *path, const char 
*fmt, ...)
va_start(args, fmt);
err = do_submodule_path(, path, fmt, args);
va_end(args);
-   if (err)
+   if (err) {
+   strbuf_release();
return NULL;
+   }
return strbuf_detach(, NULL);
 }
 
-- 
2.10.0.rc0.259.g83512d9

--
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 v14 21/27] bisect--helper: `bisect_log` shell function in C

[Junio C Hamano]

Pranit Bauva  writes:

> +static int bisect_log(void)
> +{
> + struct strbuf buf = STRBUF_INIT;
> +
> + if (strbuf_read_file(, git_path_bisect_log(), 256) < 0) {
> + strbuf_release();
> + return error(_("We are not bisecting.\n"));
> + }
> +
> + printf("%s", buf.buf);
> + strbuf_release();
> +
> + return 0;
> +}

Hmph, is it really necessary to slurp everything in a strbuf before
sending it out to the standard output?  Wouldn't it be sufficient to
open a file descriptor for reading on the log file and then hand it
over to copy.c::copy_fd()?

--
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 v11 0/8] submodule inline diff format

[Jacob Keller]

On Fri, Aug 26, 2016 at 1:04 PM, Jeff King  wrote:
> On Fri, Aug 26, 2016 at 07:58:07PM +, Keller, Jacob E wrote:
>
>> > >  char *git_pathdup_submodule(const char *path, const char *fmt,
>> > > ...)
>> > >  {
>> > > +   int err;
>> > > va_list args;
>> > > struct strbuf buf = STRBUF_INIT;
>> > > va_start(args, fmt);
>> > > -   do_submodule_path(, path, fmt, args);
>> > > +   err = do_submodule_path(, path, fmt, args);
>> > > va_end(args);
>> > > +   if (err)
>> >
>> > Here we need a strbuf_release() to avoid a memory leak?
>>
>> No, cause we "strbuf_detach" after this to return the buffer? Or is
>> that not safe?
>
> That code path is OK. I think the question is whether you need to
> release the buffer in the "err" case where you return NULL and don't hit
> the strbuf_detach.
>
> IOW, does do_submodule_path() promise that when it returns an error,
> "buf" has been left uninitialized? Some of our strbuf functions do, but
> I do not know offhand about do_submodule_path().
>
> -Peff

We probably should release for the error case. I'll do that. I don't
believe do_submodule_path ensures that the passed in argument is
guaranteed to not be initialized or used.

Thanks,
Jake
--
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: Are --first-parent and --ancestry-path compatible rev-list options?

[Junio C Hamano]

"Philip Oakley"  writes:

> The commit graph. We are looking for F based on knowing J.
>
> . A - B - C - D -- E -- F -- G - H<-first parent, --merges (C,F,H)
> .  \  |  /\//
> .   Z |   //
> . |   |   |   /
> .  \   \ /   /
> .   I -[J]- K - L - M <-since J, children of J
> .\ /
> . N - O - P

I think these two operations are fundamentally incompatible.

Because the first-parent traversal is what the name says, i.e.,
forbids the positive side of revision traversal to stray into side
branches, the positive side of a traversal that begins at H will not
see M, L and K.  The negative side would traverse in the normal way
to paint commits starting J and its ancestors as UNINTERESTING, so
the traversal over the artificial "first-parent only" history, i.e.
H, G, F, E, D, C, B, A would eventually stop before showing an
ancestor of J.

On the other hand, to compute the ancestry-path, you need to make a
full traversal of the real history to find a subgraph J..H and then
post-process it to cull paths that do not contain J.
--
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] Documentation/SubmittingPatches: add quotes to advised commit reference

[Stefan Beller]

On Fri, Aug 26, 2016 at 3:42 PM, Junio C Hamano  wrote:
>
> Perhaps something like this instead?
>
> -- >8 --

This is way better.
Let's take that.

Thanks,
Stefan

> From: Beat Bolli 
> Subject: SubmittingPatches: use gitk's "Copy commit summary" format
> Date: Fri, 26 Aug 2016 18:59:01 +0200
>
> Update the suggestion in 175d38ca ("SubmittingPatches: document how
> to reference previous commits", 2016-07-28) on the format to refer
> to a commit to match what gitk has been giving since last year with
> its "Copy commit summary" command; also mention this as one of the
> ways to obtain a commit reference in this format.
>
> Signed-off-by: Beat Bolli 
> ---
>  Documentation/SubmittingPatches | 11 ---
>  1 file changed, 8 insertions(+), 3 deletions(-)
>
> diff --git a/Documentation/SubmittingPatches b/Documentation/SubmittingPatches
> index 500230c..15adb86 100644
> --- a/Documentation/SubmittingPatches
> +++ b/Documentation/SubmittingPatches
> @@ -122,9 +122,14 @@ without external resources. Instead of giving a URL to a 
> mailing list
>  archive, summarize the relevant points of the discussion.
>
>  If you want to reference a previous commit in the history of a stable
> -branch use the format "abbreviated sha1 (subject, date)". So for example
> -like this: "Commit f86a374 (pack-bitmap.c: fix a memleak, 2015-03-30)
> -noticed [...]".
> +branch, use the format "abbreviated sha1 (subject, date)",
> +with the subject enclosed in a pair of double-quotes, like this:
> +
> +Commit f86a374 ("pack-bitmap.c: fix a memleak", 2015-03-30)
> +noticed that ...
> +
> +The "Copy commit summary" command of gitk can be used to obtain this
> +format.
>
>
>  (3) Generate your patch using Git tools out of your commits.
--
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] Documentation/SubmittingPatches: add quotes to advised commit reference

[Junio C Hamano]

Stefan Beller  writes:

> Junio finds it is easier to read text when the commit subject is quoted.
>
> Signed-off-by: Stefan Beller 
> ---
>  Documentation/SubmittingPatches | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/Documentation/SubmittingPatches b/Documentation/SubmittingPatches
> index 500230c..a591229 100644
> --- a/Documentation/SubmittingPatches
> +++ b/Documentation/SubmittingPatches
> @@ -123,7 +123,7 @@ archive, summarize the relevant points of the discussion.
>  
>  If you want to reference a previous commit in the history of a stable
>  branch use the format "abbreviated sha1 (subject, date)". So for example
> -like this: "Commit f86a374 (pack-bitmap.c: fix a memleak, 2015-03-30)
> +like this: "Commit f86a374 ("pack-bitmap.c: fix a memleak", 2015-03-30)
>  noticed [...]".

Thanks, but it is not sufficient as you would not see the need for
quoting without the example.

My preference is not a main deciding factor in this case anyway.  A
more important reason why it makes sense to quote (aside from the
fact that it makes more sense when reading) is because we already
have a tool support for that.

Perhaps something like this instead?

-- >8 --
From: Beat Bolli 
Subject: SubmittingPatches: use gitk's "Copy commit summary" format
Date: Fri, 26 Aug 2016 18:59:01 +0200

Update the suggestion in 175d38ca ("SubmittingPatches: document how
to reference previous commits", 2016-07-28) on the format to refer
to a commit to match what gitk has been giving since last year with
its "Copy commit summary" command; also mention this as one of the
ways to obtain a commit reference in this format.

Signed-off-by: Beat Bolli 
---
 Documentation/SubmittingPatches | 11 ---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/Documentation/SubmittingPatches b/Documentation/SubmittingPatches
index 500230c..15adb86 100644
--- a/Documentation/SubmittingPatches
+++ b/Documentation/SubmittingPatches
@@ -122,9 +122,14 @@ without external resources. Instead of giving a URL to a 
mailing list
 archive, summarize the relevant points of the discussion.
 
 If you want to reference a previous commit in the history of a stable
-branch use the format "abbreviated sha1 (subject, date)". So for example
-like this: "Commit f86a374 (pack-bitmap.c: fix a memleak, 2015-03-30)
-noticed [...]".
+branch, use the format "abbreviated sha1 (subject, date)",
+with the subject enclosed in a pair of double-quotes, like this:
+
+Commit f86a374 ("pack-bitmap.c: fix a memleak", 2015-03-30)
+noticed that ...
+
+The "Copy commit summary" command of gitk can be used to obtain this
+format.
 
 
 (3) Generate your patch using Git tools out of your commits.
--
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

[ANNOUNCE] Git v2.10.0-rc2

[Junio C Hamano]

A release candidate Git v2.10.0-rc2 is now available for testing
at the usual places.  It is comprised of 623 non-merge commits
since v2.9.0, contributed by 71 people, 22 of which are new faces.

The tarballs are found at:

https://www.kernel.org/pub/software/scm/git/testing/

The following public repositories all have a copy of the
'v2.10.0-rc2' tag and the 'master' branch that the tag points at:

  url = https://kernel.googlesource.com/pub/scm/git/git
  url = git://repo.or.cz/alt-git.git
  url = git://git.sourceforge.jp/gitroot/git-core/git.git
  url = git://git-core.git.sourceforge.net/gitroot/git-core/git-core
  url = https://github.com/gitster/git

New contributors whose contributions weren't in v2.9.0 are as follows.
Welcome to the Git development community!

  Alexander Hirsch, Andreas Brauchli, Andrew Oakley, Antoine Queru,
  Ben Wijen, Christopher Layne, Dave Nicolson, David Glasser, Ed
  Maste, Heiko Becker, Ingo Brückl, Jonathan Tan, Jordan DE GEA,
  Josef Kufner, Keith McGuigan, Kevin Willford, LE Manh Cuong,
  Michael Stahl, Parker Moore, Peter Colberg, Tom Russello,
  and William Duclot.

Returning contributors who helped this release are as follows.
Thanks for your continued support.

  Alex Henrie, Alfred Perlstein, Armin Kunaschik, brian m. carlson,
  Charles Bailey, Chris Packham, Christian Couder, David A. Greene,
  David Aguilar, David Kastrup, David Turner, Edward Thomson,
  Elia Pinto, Eric Sunshine, Eric Wong, Heiko Voigt, Jacob Keller,
  Jean-Noel Avila, Jeff King, Joey Hess, Johannes Schindelin,
  Johannes Sixt, John Keeping, Jonathan Nieder, Josh Triplett,
  Junio C Hamano, Lars Schneider, Lars Vogel, Linus Torvalds,
  Lukas Fleischer, Matthieu Moy, Mehul Jain, Michael Haggerty,
  Michael J Gruber, Mike Hommey, Nguyễn Thái Ngọc Duy,
  Nicolas Pitre, Orgad Shaneh, Patrick Steinhardt, Pranit Bauva,
  Ramsay Jones, René Scharfe, Ronald Wampler, Stefan Beller,
  SZEDER Gábor, Thomas Braun, Torsten Bögershausen, Vasco
  Almeida, and Ville Skyttä.



Git 2.10 Release Notes (draft)
==

Backward compatibility notes


Updates since v2.9
--

UI, Workflows & Features

 * "git pull --rebase --verify-signature" learned to warn the user
   that "--verify-signature" is a no-op when rebasing.

 * An upstream project can make a recommendation to shallowly clone
   some submodules in the .gitmodules file it ships.

 * "git worktree add" learned that '-' can be used as a short-hand for
   "@{-1}", the previous branch.

 * Update the funcname definition to support css files.

 * The completion script (in contrib/) learned to complete "git
   status" options.

 * Messages that are generated by auto gc during "git push" on the
   receiving end are now passed back to the sending end in such a way
   that they are shown with "remote: " prefix to avoid confusing the
   users.

 * "git add -i/-p" learned to honor diff.compactionHeuristic
   experimental knob, so that the user can work on the same hunk split
   as "git diff" output.

 * "upload-pack" allows a custom "git pack-objects" replacement when
   responding to "fetch/clone" via the uploadpack.packObjectsHook.
   (merge b738396 jk/upload-pack-hook later to maint).

 * Teach format-patch and mailsplit (hence "am") how a line that
   happens to begin with "From " in the e-mail message is quoted with
   ">", so that these lines can be restored to their original shape.
   (merge d9925d1 ew/mboxrd-format-am later to maint).

 * "git repack" learned the "--keep-unreachable" option, which sends
   loose unreachable objects to a pack instead of leaving them loose.
   This helps heuristics based on the number of loose objects
   (e.g. "gc --auto").
   (merge e26a8c4 jk/repack-keep-unreachable later to maint).

 * "log --graph --format=" learned that "%>|(N)" specifies the width
   relative to the terminal's left edge, not relative to the area to
   draw text that is to the right of the ancestry-graph section.  It
   also now accepts negative N that means the column limit is relative
   to the right border.

 * A careless invocation of "git send-email directory/" after editing
   0001-change.patch with an editor often ends up sending both
   0001-change.patch and its backup file, 0001-change.patch~, causing
   embarrassment and a minor confusion.  Detect such an input and
   offer to skip the backup files when sending the patches out.
   (merge 531220b jc/send-email-skip-backup later to maint).

 * "git submodule update" that drives many "git clone" could
   eventually hit flaky servers/network conditions on one of the
   submodules; the command learned to retry the attempt.

 * The output coloring scheme learned two new attributes, italic and
   strike, in addition to existing bold, reverse, etc.

 * "git log" learns log.showSignature configuration variable, and a
   command line option "--no-show-signature" to 

Re: [PATCH 1/2] gitk: align the commit summary format to the documentation

[Stefan Beller]

On Fri, Aug 26, 2016 at 2:27 PM, Junio C Hamano  wrote:
> Beat Bolli  writes:
>
>> On 26.08.16 21:16, Stefan Beller wrote:
>>> I agree we should fix that.
>>
>> So would you prepare a amendment to your documentation commit so that
>> Junio can disregard my two patches?
>
> I think the mention of gitk having a feature to easily give you a
> commit name in the preferred format added by your 2/2 is worth
> keeping.  I am not Stefan, though ;-)

I just sent a patch to the list to fix the formatting in SubmittingPatches.
Please keep 2/2, though ?

Thanks,
Stefan
--
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] Documentation/SubmittingPatches: add quotes to advised commit reference

[Stefan Beller]

Junio finds it is easier to read text when the commit subject is quoted.

Signed-off-by: Stefan Beller 
---
 Documentation/SubmittingPatches | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Documentation/SubmittingPatches b/Documentation/SubmittingPatches
index 500230c..a591229 100644
--- a/Documentation/SubmittingPatches
+++ b/Documentation/SubmittingPatches
@@ -123,7 +123,7 @@ archive, summarize the relevant points of the discussion.
 
 If you want to reference a previous commit in the history of a stable
 branch use the format "abbreviated sha1 (subject, date)". So for example
-like this: "Commit f86a374 (pack-bitmap.c: fix a memleak, 2015-03-30)
+like this: "Commit f86a374 ("pack-bitmap.c: fix a memleak", 2015-03-30)
 noticed [...]".
 
 
-- 
2.10.0.rc1.1.g1ceb01a

--
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] transport: report missing submodule pushes consistently on stderr

[Stefan Beller]

On Wed, Aug 24, 2016 at 9:35 AM, Stefan Beller  wrote:
> On Wed, Aug 24, 2016 at 3:28 AM, Leandro Lucarella
>  wrote:
>> On Tue, 23 Aug 2016 14:40:08 -0700
>> Stefan Beller  wrote:
>>> The surrounding advice is printed to stderr, but the list of
>>> submodules is not. Make the report consistent by reporting everything
>>> to stderr.
>>>
>>> Signed-off-by: Stefan Beller 
>>> ---

So we seem to have dropped the ball on the followup; this patch nevertheless
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 1/2] gitk: align the commit summary format to the documentation

[Junio C Hamano]

Beat Bolli  writes:

> On 26.08.16 21:16, Stefan Beller wrote:
>> I agree we should fix that.
>
> So would you prepare a amendment to your documentation commit so that
> Junio can disregard my two patches?

I think the mention of gitk having a feature to easily give you a
commit name in the preferred format added by your 2/2 is worth
keeping.  I am not Stefan, though ;-)
--
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

Are --first-parent and --ancestry-path compatible rev-list options?

[Philip Oakley]

While trying to answer a Stack Overflow question I thought I could 
contribute to, I've found a scenario that I don't understand that may be a 
bug.


In http://stackoverflow.com/questions/39144006/identify-merge-into-master 
MvG asked how to find the point at which a commit on a feature branch was 
later merged into the main-line.


After some discussion it appeared that
   `git log --oneline  --first-parent --merges --reverse  --ancestry-path 
:/j..  | head -1`
should find the point that 'j' was merged into master (when on master), 
however it appears that --first-parent and --ancestry-path interact badly to 
produce no output in the example, but if either is dropped, the expected 
commit is shown.


What am I missing?
--
Philip


The commit graph. We are looking for F based on knowing J.

. A - B - C - D -- E -- F -- G - H<-first parent, --merges (C,F,H)
.  \  |  /\//
.   Z |   //
. |   |   |   /
.  \   \ /   /
.   I -[J]- K - L - M <-since J, children of J
.\ /
. N - O - P

# Steps to reproduce the extended example from
# http://stackoverflow.com/q/39144006/1468366

git init .
echo a > txt
git add txt
git commit -m a
echo a > txt; git commit -a -m a
echo b > txt; git commit -a -m b
git checkout -b side :/a
echo z > txt; git commit -a -m z
git checkout master
git merge :/z; echo c > txt; git add -u; git commit -m c

#echo c > txt; git commit -a -m c
echo d > txt; git commit -a -m d
echo e > txt; git commit -a -m e
git checkout -b 2nd :/b
echo i > txt; git commit -a -m i
echo j > txt; git commit -a -m j
git merge :/d; echo k > txt; git add -u; git commit -m k
git checkout -b 3rd :/i
echo n > txt; git commit -a -m n
echo o > txt; git commit -a -m o
echo p > txt; git commit -a -m p
git checkout 2nd
git merge :/p; echo l > txt; git add -u; git commit -m l
echo m > txt; git commit -a -m m
git checkout master
git merge :/l; echo f > txt; git add -u; git commit -m f
git merge :/m; echo g > txt; git add -u; git commit -m g
echo h > txt; git commit -a -m h

git log --oneline  --first-parent --merges --reverse  --ancestry-path :/j.. 
| head -5


# why does this not work --ancestry-path and --first-parent appear to clash.

code available as 
https://gist.github.com/PhilipOakley/58f344f910e50b72f5a8a2bd55b6c175


--
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 v14 17/27] bisect--helper: `bisect_autostart` shell function in C

[Junio C Hamano]

Pranit Bauva  writes:

> @@ -410,6 +413,7 @@ static int bisect_next(struct bisect_terms *terms, const 
> char *prefix)
>  {
>   int res, no_checkout;
>  
> + bisect_autostart(terms);
>   /* In case of mistaken revs or checkout error, or signals received,
>* "bisect_auto_next" below may exit or misbehave.
>* We have to trap this to be able to clean up using

This change is extremely hard to reason about.

Why wasn't auto-start done at the very beginning of this function
before?  Why does this start calling it all of a sudden?  Before
autostart was rewritten in C in this step, who made sure the shell
version of autostart was called before bisect_next is called (which
must have become unnecessary with this step, and I fully expected a
removal of such a call in the shell version, but I didn't find such
a change in this patch)?

Or is this series simply broken by lossage of autostart feature in
"bisect next" between the step where bisect_next is rewritten in C
and this step?

--
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 2/2] gitattributes: Document the unified "auto" handling

[Junio C Hamano]

tbo...@web.de writes:

> From: Torsten Bögershausen 
>
> Update the documentation about text=auto:
> text=auto now follows the core.autocrlf handling when files are not
> normalized in the repository.
>
> For a cross platform project recommend the usage of attributes for
> line-ending conversions.
>
> Signed-off-by: Torsten Bögershausen 
> ---
>  Documentation/gitattributes.txt | 58 
> +
>  1 file changed, 24 insertions(+), 34 deletions(-)

Looks good.  Let me merge this to 'master' before cutting -rc2.

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 v14 15/27] bisect--helper: retire `--bisect-clean-state` subcommand

[Junio C Hamano]

Pranit Bauva  writes:

> The `bisect-clean-state` subcommand is no longer used in the shell
> script while the C code uses `bisect_clean_state()` thus remove the
> subcommand.

Good.
--
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] gitk: align the commit summary format to the documentation

[Beat Bolli]

On 26.08.16 21:16, Stefan Beller wrote:
> On Fri, Aug 26, 2016 at 11:24 AM, Junio C Hamano  wrote:
>> Beat Bolli  writes:
>>
>>> In 175d38c (SubmittingPatches: document how to reference previous commits,
>>> 2016-07-28) the format for referring to older commits was specified.
>>>
>>> Make the text generated by the "Copy commit summary" command match this
>>> format.
>>
>> Hmph.  I didn't know gitk already had its own command to produce a
>> short string.  I actually think what it produces
> 
> It was added in d835dbb91fe (gitk: Add a "Copy commit summary" command,
> 2015-07-18), it doesn't seem to be in your tree yet, so maybe wait
> with this patch
> until you pulled gitk?

This commit was part of release 2.6.0.

>>> In 175d38c ("SubmittingPatches: document how to reference previous commits",
>>> 2016-07-28) the format for referring to older commits was specified.
>>
>> is easier to read when pasted into a sentence than what the recent
>> update 175d38ca ("SubmittingPatches: document how to reference
>> previous commits", 2016-07-28) suggests to do, i.e.
>>
>>> In 175d38c (SubmittingPatches: document how to reference previous commits,
>>> 2016-07-28) the format for referring to older commits was specified.
>>
>> Heiko, Stefan, I think you two were involved in adding that new
>> paragraph.   What do you think?
> 
> So the subtle difference is adding '"' around the commit message subject?
> 
> I agree we should fix that.

So would you prepare a amendment to your documentation commit so that
Junio can disregard my two patches?

Thanks,
Beat
--
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 2/3] help: introduce option --exclude-guides

[Ralf Thielow]

2016-08-26 22:20 GMT+02:00 Junio C Hamano :
>
> Because the whole thing is inside a double-quote pair, $() and $name
> are all interpolated even before test_expect_success is called.
> So the above becomes equivalent to
>
>>> test_expect_success "two commits do not have the same ID" '
>>> git commit --allow-empty -m first &&
>>> one=5cb0d5ad05e027cbddcb0a3c7518ddeea0f7c286 &&
>>> test_tick &&
>>> git commit --allow-empty -m second &&
>>> two=5cb0d5ad05e027cbddcb0a3c7518ddeea0f7c286 &&
>>> test !=
>>> '
>

I got it, thanks.  My understanding in when a part is being interpreted
was obviously very wrong.  Thanks again!
--
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 2/3] help: introduce option --exclude-guides

[Junio C Hamano]

Ralf Thielow  writes:

> 2016-08-26 21:42 GMT+02:00 Junio C Hamano :
>> Junio C Hamano  writes:
>>
>>
>> Taking all of these together, I'll queue this as a proposed fix-up
>> directly on top of yours.
>>
>
> Thanks!

Thank you for starting this topic.  I forgot to add comment on that
test://html part, though, so I'd have to redo it further, but
perhaps not today.
--
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 0/2] Adjust the documentation to the unified "auto" handling

[tboegi]

From: Torsten Bögershausen 

Changes since v1:
- 1/2 is left unchanged
- 2/2 is re-written and should be more consistant to read.

Torsten Bögershausen (2):
  git ls-files: text=auto eol=lf is supported in Git 2.10
  gitattributes: Document the unified "auto" handling

 Documentation/git-ls-files.txt  |  3 +--
 Documentation/gitattributes.txt | 58 +
 2 files changed, 25 insertions(+), 36 deletions(-)

-- 
2.9.0.243.g5c589a7

--
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 2/2] gitattributes: Document the unified "auto" handling

[tboegi]

From: Torsten Bögershausen 

Update the documentation about text=auto:
text=auto now follows the core.autocrlf handling when files are not
normalized in the repository.

For a cross platform project recommend the usage of attributes for
line-ending conversions.

Signed-off-by: Torsten Bögershausen 
---
 Documentation/gitattributes.txt | 58 +
 1 file changed, 24 insertions(+), 34 deletions(-)

diff --git a/Documentation/gitattributes.txt b/Documentation/gitattributes.txt
index 807577a..7aff940 100644
--- a/Documentation/gitattributes.txt
+++ b/Documentation/gitattributes.txt
@@ -182,23 +182,6 @@ While Git normally leaves file contents alone, it can be 
configured to
 normalize line endings to LF in the repository and, optionally, to
 convert them to CRLF when files are checked out.
 
-Here is an example that will make Git normalize .txt, .vcproj and .sh
-files, ensure that .vcproj files have CRLF and .sh files have LF in
-the working directory, and prevent .jpg files from being normalized
-regardless of their content.
-
-
-*   text=auto
-*.txt  text
-*.vcproj   text eol=crlf
-*.sh   text eol=lf
-*.jpg  -text
-
-
-Other source code management systems normalize all text files in their
-repositories, and there are two ways to enable similar automatic
-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 using any attributes.
@@ -208,35 +191,42 @@ config variable "core.autocrlf" without using any 
attributes.
autocrlf = true
 
 
-This does not force normalization of all text files, but does ensure
+This does not force normalization of text files, but does ensure
 that text files that you introduce to the repository have their line
 endings normalized to LF when they are added, and that files that are
 already normalized in the repository stay normalized.
 
-If you want to interoperate with a source code management system that
-enforces end-of-line normalization, or you simply want all text files
-in your repository to be normalized, you should instead set the `text`
-attribute to "auto" for _all_ files.
+If you want to ensure that text files that any contributor introduces to
+the repository have their line endings normalized, you can set the
+`text` attribute to "auto" for _all_ files.
 
 
 *  text=auto
 
 
-This ensures that all files that Git considers to be text will have
-normalized (LF) line endings in the repository.  The `core.eol`
-configuration variable controls which line endings Git will use for
-normalized files in your working directory; the default is to use the
-native line ending for your platform, or CRLF if `core.autocrlf` is
-set.
+The attributes allow a fine-grained control, how the line endings
+are converted.
+Here is an example that will make Git normalize .txt, .vcproj and .sh
+files, ensure that .vcproj files have CRLF and .sh files have LF in
+the working directory, and prevent .jpg files from being normalized
+regardless of their content.
+
+
+*   text=auto
+*.txt  text
+*.vcproj   text eol=crlf
+*.sh   text eol=lf
+*.jpg  -text
+
+
+NOTE: When `text=auto` conversion is enabled in a cross-platform
+project using push and pull to a central repository the text files
+containing CRLFs should be normalized.
 
-NOTE: When `text=auto` normalization is enabled in an existing
-repository, any text files containing CRLFs should be normalized.  If
-they are not they will be normalized the next time someone tries to
-change them, causing unfortunate misattribution.  From a clean working
-directory:
+From a clean working directory:
 
 -
-$ echo "* text=auto" >>.gitattributes
+$ echo "* text=auto" >.gitattributes
 $ rm .git/index # Remove the index to force Git to
 $ git reset # re-scan the working directory
 $ git status# Show files that will be normalized
-- 
2.9.0.243.g5c589a7

--
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 1/2] git ls-files: text=auto eol=lf is supported in Git 2.10

[tboegi]

From: Torsten Bögershausen 

The man page for `git ls-files --eol` mentions the combination
of text attributes "text=auto eol=lf" or "text=auto eol=crlf" as not
supported yet, but may be in the future.
Now they are supported

Signed-off-by: Torsten Bögershausen 
---
 Documentation/git-ls-files.txt | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/Documentation/git-ls-files.txt b/Documentation/git-ls-files.txt
index 078b556..0d933ac 100644
--- a/Documentation/git-ls-files.txt
+++ b/Documentation/git-ls-files.txt
@@ -159,8 +159,7 @@ not accessible in the working tree.
 +
  is the attribute that is used when checking out or committing,
 it is either "", "-text", "text", "text=auto", "text eol=lf", "text eol=crlf".
-Note: Currently Git does not support "text=auto eol=lf" or "text=auto 
eol=crlf",
-that may change in the future.
+Since Git 2.10 "text=auto eol=lf" and "text=auto eol=crlf" are supported.
 +
 Both the  in the index ("i/")
 and in the working tree ("w/") are shown for regular files,
-- 
2.9.0.243.g5c589a7

--
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 2/3] help: introduce option --exclude-guides

[Junio C Hamano]

Ralf Thielow  writes:

>>> As we pass a URL, Git won't check if the given path looks like
>>> a documentation directory.  Another solution would be to create
>>> a directory, add a file "git.html" to it and just use this path.
>>
>> I think this is OK; with s|As we pass a URL|As we pass a string with
>> :// in it|, the first sentence can be a in-code comment in the test
>> that does this and will help readers of the code in the future.
>
> Hmm. The "://" is really a URL thing.

Perhaps you thought so, but no, "mailto:ralf.thie...@gmail.com; is a
perfectly valid URL.

Because you are explaining why test://html was chosen, and the real
reason is any path that is !strstr(path, "://") is subject to an
additional "This must be a local path" check and you wanted to avoid
it, "As we pass a URL" is unnecessarily vague (and incorrect--we
cannot use a mailto: URL to sidestep the check).

>> *1* Can you immediately tell why this test is broken?
>>
>> test_expect_success "two commits do not have the same ID" "
>> git commit --allow-empty -m first &&
>> one=$(git rev-parse --verify HEAD) &&
>> test_tick &&
>> git commit --allow-empty -m second &&
>> two=$(git rev-parse --verify HEAD) &&
>> test $one != $two
>> "
>>
>
> I'm afraid I can't.

The reason becomes clear if you put your feet into shell's shues.
Before being ablt to call test_expect_success, you would need to
figure out what strings you give as its parameters.  $1 is clear in
this case, a simple string "two commits do not have the same ID"
(without double quotes).

But what goes in $2?  Especially the part around "one=..."?

Because the whole thing is inside a double-quote pair, $() and $name
are all interpolated even before test_expect_success is called.
So the above becomes equivalent to

>> test_expect_success "two commits do not have the same ID" '
>> git commit --allow-empty -m first &&
>> one=5cb0d5ad05e027cbddcb0a3c7518ddeea0f7c286 &&
>> test_tick &&
>> git commit --allow-empty -m second &&
>> two=5cb0d5ad05e027cbddcb0a3c7518ddeea0f7c286 &&
>> test !=
>> '

(using whatever commit HEAD was pointing at before this test starts
to run), which obviously is not what we expected to see.

--
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 v6 09/13] convert: modernize tests

[Junio C Hamano]

larsxschnei...@gmail.com writes:

> From: Lars Schneider 
>
> Use `test_config` to set the config, check that files are empty with
> `test_must_be_empty`, compare files with `test_cmp`, and remove spaces
> after ">" and "<".

All of the above are good things to do, but the first one needs to
be done a bit carefully.

It is unclear in the above description if you made sure that no
subsequent test depends on the settings left by earlier test before
replacing "git config" with "test_config".

> diff --git a/t/t0021-conversion.sh b/t/t0021-conversion.sh
> index 7bac2bc..7b45136 100755
> --- a/t/t0021-conversion.sh
> +++ b/t/t0021-conversion.sh
> @@ -13,8 +13,8 @@ EOF
>  chmod +x rot13.sh
>  
>  test_expect_success setup '
> - git config filter.rot13.smudge ./rot13.sh &&
> - git config filter.rot13.clean ./rot13.sh &&
> + test_config filter.rot13.smudge ./rot13.sh &&
> + test_config filter.rot13.clean ./rot13.sh &&
>  
>   {
>   echo "*.t filter=rot13"

For example, after this conversion, filter.rot13.* will be reverted
back to unconfigured once setup is done.

>  test_expect_success check '
>  
> - cmp test.o test &&
> - cmp test.o test.t &&
> + test_cmp test.o test &&
> + test_cmp test.o test.t &&
>  
>   # ident should be stripped in the repository
>   git diff --raw --exit-code :test :test.i &&

It happens to be true that this subsequent test does not do any
operation to cause data come from and go into the object database
for any path that match the pattern "*.t", because for whatever
reason the previous "setup" step happens to do more than just
"setup".  It already exercised the filtering by doing "git add" and
"git checkout".

If we were writing the script t0021 from scratch today, we would
have used test_config *AND* squashed there two tests into one
(i.e. making it a single "the filter and ident operation" test).
Then it is crystal clear that additional tests on commands that may
involve filtering will always be added to that combined test
(e.g. we may try "cat-file --filters" to ensure that rot13 filter is
excercised).

But because we are not doing that, it may become tempting to add
test for a new command that pays attention to a filter to either of
the test, and it would have worked OK if this patch weren't there.
Such an addition will break the test, because in the second "check"
test, the filter commands are no longer active with this patch.

So while I do appreciate the change for the longer term, I am not
quite happy with it.  It just looks like an invitation for future
mistakes.

> @@ -130,7 +130,7 @@ test_expect_success 'filter shell-escaped filenames' '
>  
>   # delete the files and check them out again, using a smudge filter
>   # that will count the args and echo the command-line back to us
> - git config filter.argc.smudge "sh ./argc.sh %f" &&
> + test_config filter.argc.smudge "sh ./argc.sh %f" &&
>   rm "$normal" "$special" &&
>   git checkout -- "$normal" "$special" &&

This one is clearly OK.  Anything related to argc filter only
appears in this single test so it is not just OK to use test_config,
but it makes our intention very clear that nobody else would use the
argc filter.  I think similar reasoning would apply to the test_config
conversion in the remainder of the script.

As to other types of changes you did in this, I see no problem with
them.

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 v2 2/3] help: introduce option --exclude-guides

[Ralf Thielow]

2016-08-26 21:42 GMT+02:00 Junio C Hamano :
> Junio C Hamano  writes:
>
>
> Taking all of these together, I'll queue this as a proposed fix-up
> directly on top of yours.
>

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 v11 0/8] submodule inline diff format

[Jeff King]

On Fri, Aug 26, 2016 at 07:58:07PM +, Keller, Jacob E wrote:

> > >  char *git_pathdup_submodule(const char *path, const char *fmt,
> > > ...)
> > >  {
> > > +   int err;
> > > va_list args;
> > > struct strbuf buf = STRBUF_INIT;
> > > va_start(args, fmt);
> > > -   do_submodule_path(, path, fmt, args);
> > > +   err = do_submodule_path(, path, fmt, args);
> > > va_end(args);
> > > +   if (err)
> > 
> > Here we need a strbuf_release() to avoid a memory leak?
> 
> No, cause we "strbuf_detach" after this to return the buffer? Or is
> that not safe?

That code path is OK. I think the question is whether you need to
release the buffer in the "err" case where you return NULL and don't hit
the strbuf_detach.

IOW, does do_submodule_path() promise that when it returns an error,
"buf" has been left uninitialized? Some of our strbuf functions do, but
I do not know offhand about do_submodule_path().

-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: [PATCH v2 2/3] help: introduce option --exclude-guides

[Ralf Thielow]

2016-08-26 21:06 GMT+02:00 Junio C Hamano :
> Ralf Thielow  writes:
>
>> Introduce option --exclude-guides to the help command.  With this option
>> being passed, "git help" will open man pages only for actual commands.
>
> Let's hide this option from command help of "git help" itself, drop
> the short-and-sweet "-e", not command-line complete it, and leave it
> not-mentioned here.
>
> It is a different story if this option would really help the end
> users, but I do not think that is the case.  If this were to face
> the end users properly, we would need to worry about making sure
> that "git help -g -e" would error out, and getting all the other
> possible corner cases right.  I do not think the amount of effort
> required to do so (even the "trying to enumerate what other possible
> corner cases there may be" part) is worth it.
>

I'm fine with that as the reason for me was just a "why not?", and
you just gave the reason to not do this. Thanks

>> In the test script we do two things I'd like to point out:
>>
>>> +   test_config help.htmlpath test://html &&
>>
>> As we pass a URL, Git won't check if the given path looks like
>> a documentation directory.  Another solution would be to create
>> a directory, add a file "git.html" to it and just use this path.
>
> I think this is OK; with s|As we pass a URL|As we pass a string with
> :// in it|, the first sentence can be a in-code comment in the test
> that does this and will help readers of the code in the future.
>

Hmm. The "://" is really a URL thing. That's why it's in the code, no?
The code may have some room for improvements in checking for
URLs.

>>> +   test_config help.browser firefox
>>
>> Git checks if the browser is known, so the "test-browser" needs to
>> pretend it is one of them.
>
> Are you talking about the hardcoded list in valid_tool() helper
> function in git-web--browse.sh?  If we use the established escape
> hatch implemented by valid_custom_tool() helper there by setting
> browser.*.cmd, would that be sufficient to work around the "Git
> checks if the browser is known"?
>
>> diff --git a/Documentation/git-help.txt b/Documentation/git-help.txt
>> index 40d328a..eeb1950 100644
>> --- a/Documentation/git-help.txt
>> +++ b/Documentation/git-help.txt
>> @@ -8,7 +8,7 @@ git-help - Display help information about Git
>>  SYNOPSIS
>>  
>>  [verse]
>> -'git help' [-a|--all] [-g|--guide]
>> +'git help' [-a|--all] [-e|--exclude-guides] [-g|--guide]
>>  [-i|--info|-m|--man|-w|--web] [COMMAND|GUIDE]
>
> So, let's not do this.
>
>> diff --git a/builtin/help.c b/builtin/help.c
>> index e8f79d7..40901a9 100644
>> --- a/builtin/help.c
>> +++ b/builtin/help.c
>> @@ -37,8 +37,10 @@ static int show_all = 0;
>>  static int show_guides = 0;
>>  static unsigned int colopts;
>>  static enum help_format help_format = HELP_FORMAT_NONE;
>> +static int exclude_guides;
>>  static struct option builtin_help_options[] = {
>>   OPT_BOOL('a', "all", _all, N_("print all available commands")),
>> + OPT_BOOL('e', "exclude-guides", _guides, N_("exclude guides")),
>
> So I'd suggest using PARSE_OPT_HIDDEN for this one and drop 'e' shorthand.
> The only caller of this mode does not use it.
>
>> diff --git a/contrib/completion/git-completion.bash 
>> b/contrib/completion/git-completion.bash
>> index c1b2135..b148164 100644
>> --- a/contrib/completion/git-completion.bash
>> +++ b/contrib/completion/git-completion.bash
>> @@ -1393,7 +1393,7 @@ _git_help ()
>>  {
>>   case "$cur" in
>>   --*)
>> - __gitcomp "--all --guides --info --man --web"
>> + __gitcomp "--all --exclude-guides --guides --info --man --web"
>>   return
>>   ;;
>>   esac
>
> So, let's not do this.
>
>> diff --git a/t/t0012-help.sh b/t/t0012-help.sh
>> new file mode 100755
>> index 000..fb1abd7
>> --- /dev/null
>> +++ b/t/t0012-help.sh
>> @@ -0,0 +1,33 @@
>> +#!/bin/sh
>> +
>> +test_description='help'
>> +
>> +. ./test-lib.sh
>> +
>> +configure_help () {
>> + test_config help.format html &&
>> + test_config help.htmlpath test://html &&
>> + test_config help.browser firefox
>> +}
>
> Would replacing the last line with:
>
> test_config browser.test.cmd ./test-browser &&
> test_config help.browser test
>
> and then writing to test-browser work just as well?  If so, that
> would be much cleaner and more preferrable.
>

I wasn't aware that this is a way to configure things. Thanks.

>> +
>> +test_expect_success "setup" "
>> + write_script firefox <<-\EOF
>> + exit 0
>> + EOF
>> +"
>
> Unless there is a good reason you MUST do so, avoid quoting the test
> body with double quotes, as it invites mistakes [*1*].
>

The test-browser was supposed to be returning just a success
which is good enough for my usage.

> Also, how about using something like:
>
> write_script test-browser <<-\EOF
> i=0
> for arg
> do
>   

Re: [PATCH v11 0/8] submodule inline diff format

[Keller, Jacob E]

On Fri, 2016-08-26 at 12:17 -0700, Stefan Beller wrote:
> On Thu, Aug 25, 2016 at 4:32 PM, Jacob Keller  om> wrote:
> 
> > 
> > @@ -487,12 +490,14 @@ static void do_submodule_path(struct strbuf
> > *buf, const char *path,
> > strbuf_addstr(buf, git_dir);
> > }
> > if (!is_git_directory(buf->buf)) {
> > +   gitmodules_config();
> 
> We determined via chat that calling gitmodules_config
> is not harmful w.r.t. correctness, but might require some
> improvements in the future for performance.
> (i.e. we may want to add in a later series a
> if (already called gitmodules_config)
>   set flag "already called gitmodules_config";
>   return;
> into gitmodules_config)
> 
> > 
> > 
> >  char *git_pathdup_submodule(const char *path, const char *fmt,
> > ...)
> >  {
> > +   int err;
> > va_list args;
> > struct strbuf buf = STRBUF_INIT;
> > va_start(args, fmt);
> > -   do_submodule_path(, path, fmt, args);
> > +   err = do_submodule_path(, path, fmt, args);
> > va_end(args);
> > +   if (err)
> 
> Here we need a strbuf_release() to avoid a memory leak?

No, cause we "strbuf_detach" after this to return the buffer? Or is
that not safe?

Thanks,
Jake

> --
> 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
N�r��yb�X��ǧv�^�)޺{.n�+ا���ܨ}���Ơz�:+v���zZ+��+zf���h���~i���z��w���?�&�)ߢf

Re: [PATCH v6 07/13] pack-protocol: fix maximum pkt-line size

[Junio C Hamano]

Lars Schneider  writes:

>> And as this is a strict bug fix of Documentation that makes sense
>> outside this series,
>
> Agreed!

Amen.
--
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 v6 08/13] convert: quote filter names in error messages

[Junio C Hamano]

larsxschnei...@gmail.com writes:

> From: Lars Schneider 
>
> Git filter driver commands with spaces (e.g. `filter.sh foo`) are hard to
> read in error messages. Quote them to improve the readability.

Sounds good.  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 v2 2/3] help: introduce option --exclude-guides

[Junio C Hamano]

Junio C Hamano  writes:

> Let's hide this option from command help of "git help" itself, drop
> the short-and-sweet "-e", not command-line complete it, and leave it
> not-mentioned here.
> ...
> Unless there is a good reason you MUST do so, avoid quoting the test
> body with double quotes, as it invites mistakes [*1*].
>
> Also, how about using something like:
> ...
> instead?  That way, you can ensure that "git help status" attempts
> to call git-status.html with the expected path, not gitstatus.html
> or status.html, or somesuch, immediately after running "git help
> status" in the next test by inspecting test-browser.log ...

Taking all of these together, I'll queue this as a proposed fix-up
directly on top of yours.

 Documentation/git-help.txt |  6 +-
 builtin/help.c |  2 +-
 contrib/completion/git-completion.bash |  2 +-
 t/t0012-help.sh| 33 ++---
 4 files changed, 21 insertions(+), 22 deletions(-)

diff --git a/Documentation/git-help.txt b/Documentation/git-help.txt
index eeb1950..40d328a 100644
--- a/Documentation/git-help.txt
+++ b/Documentation/git-help.txt
@@ -8,7 +8,7 @@ git-help - Display help information about Git
 SYNOPSIS
 
 [verse]
-'git help' [-a|--all] [-e|--exclude-guides] [-g|--guide]
+'git help' [-a|--all] [-g|--guide]
   [-i|--info|-m|--man|-w|--web] [COMMAND|GUIDE]
 
 DESCRIPTION
@@ -43,10 +43,6 @@ OPTIONS
Prints all the available commands on the standard output. This
option overrides any given command or guide name.
 
--e::
---exclude-guides::
-   Do not show help for guides.
-
 -g::
 --guides::
Prints a list of useful guides on the standard output. This
diff --git a/builtin/help.c b/builtin/help.c
index 40901a9..49f7a07 100644
--- a/builtin/help.c
+++ b/builtin/help.c
@@ -40,7 +40,7 @@ static enum help_format help_format = HELP_FORMAT_NONE;
 static int exclude_guides;
 static struct option builtin_help_options[] = {
OPT_BOOL('a', "all", _all, N_("print all available commands")),
-   OPT_BOOL('e', "exclude-guides", _guides, N_("exclude guides")),
+   OPT_HIDDEN_BOOL(0, "exclude-guides", _guides, N_("exclude 
guides")),
OPT_BOOL('g', "guides", _guides, N_("print list of useful 
guides")),
OPT_SET_INT('m', "man", _format, N_("show man page"), 
HELP_FORMAT_MAN),
OPT_SET_INT('w', "web", _format, N_("show manual in web browser"),
diff --git a/contrib/completion/git-completion.bash 
b/contrib/completion/git-completion.bash
index 63cccb9..bd25b0a 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -1340,7 +1340,7 @@ _git_help ()
 {
case "$cur" in
--*)
-   __gitcomp "--all --exclude-guides --guides --info --man --web"
+   __gitcomp "--all --guides --info --man --web"
return
;;
esac
diff --git a/t/t0012-help.sh b/t/t0012-help.sh
index f91088b..9d99812 100755
--- a/t/t0012-help.sh
+++ b/t/t0012-help.sh
@@ -7,27 +7,30 @@ test_description='help'
 configure_help () {
test_config help.format html &&
test_config help.htmlpath test://html &&
-   test_config help.browser firefox
+   test_config browser.test.cmd ./test-browser &&
+   test_config help.browser test
 }
 
-test_expect_success "setup" "
-   write_script firefox <<-\EOF
-   exit 0
+test_expect_success "setup" '
+   write_script test-browser <<-\EOF
+   echo "$*" >test-browser.log
EOF
-"
+'
 
-test_expect_success "works for commands and guides by default" "
+test_expect_success "works for commands and guides by default" '
configure_help &&
git help status &&
-   git help revisions
-"
+   echo "test://html/git-status.html" >expect &&
+   test_cmp expect test-browser.log &&
+   git help revisions &&
+   echo "test://html/gitrevisions.html" >expect &&
+   test_cmp expect test-browser.log
+'
 
-test_expect_success "--exclude-guides does not work for guides" "
-   cat <<-EOF >expected &&
-   git: 'revisions' is not a git command. See 'git --help'.
-   EOF
-   test_must_fail git help --exclude-guides revisions 2>actual &&
-   test_i18ncmp expected actual
-"
+test_expect_success "--exclude-guides does not work for guides" '
+   >test-browser.log &&
+   test_must_fail git help --exclude-guides revisions &&
+   test_must_be_empty test-browser.log
+'
 
 test_done
-- 
2.10.0-rc1-260-gbdd1a2a

--
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 v11 0/8] submodule inline diff format

[Stefan Beller]

On Thu, Aug 25, 2016 at 4:32 PM, Jacob Keller  wrote:

> @@ -487,12 +490,14 @@ static void do_submodule_path(struct strbuf *buf, const 
> char *path,
> strbuf_addstr(buf, git_dir);
> }
> if (!is_git_directory(buf->buf)) {
> +   gitmodules_config();

We determined via chat that calling gitmodules_config
is not harmful w.r.t. correctness, but might require some
improvements in the future for performance.
(i.e. we may want to add in a later series a
if (already called gitmodules_config)
  set flag "already called gitmodules_config";
  return;
into gitmodules_config)

>
>  char *git_pathdup_submodule(const char *path, const char *fmt, ...)
>  {
> +   int err;
> va_list args;
> struct strbuf buf = STRBUF_INIT;
> va_start(args, fmt);
> -   do_submodule_path(, path, fmt, args);
> +   err = do_submodule_path(, path, fmt, args);
> va_end(args);
> +   if (err)

Here we need a strbuf_release() to avoid a memory leak?
--
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] gitk: align the commit summary format to the documentation

[Stefan Beller]

On Fri, Aug 26, 2016 at 11:24 AM, Junio C Hamano  wrote:
> Beat Bolli  writes:
>
>> In 175d38c (SubmittingPatches: document how to reference previous commits,
>> 2016-07-28) the format for referring to older commits was specified.
>>
>> Make the text generated by the "Copy commit summary" command match this
>> format.
>
> Hmph.  I didn't know gitk already had its own command to produce a
> short string.  I actually think what it produces

It was added in d835dbb91fe (gitk: Add a "Copy commit summary" command,
2015-07-18), it doesn't seem to be in your tree yet, so maybe wait
with this patch
until you pulled gitk?

>
>> In 175d38c ("SubmittingPatches: document how to reference previous commits",
>> 2016-07-28) the format for referring to older commits was specified.
>
> is easier to read when pasted into a sentence than what the recent
> update 175d38ca ("SubmittingPatches: document how to reference
> previous commits", 2016-07-28) suggests to do, i.e.
>
>> In 175d38c (SubmittingPatches: document how to reference previous commits,
>> 2016-07-28) the format for referring to older commits was specified.
>
> Heiko, Stefan, I think you two were involved in adding that new
> paragraph.   What do you think?

So the subtle difference is adding '"' around the commit message subject?

I agree we should fix that.
--
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 2/3] help: introduce option --exclude-guides

[Junio C Hamano]

Ralf Thielow  writes:

> Introduce option --exclude-guides to the help command.  With this option
> being passed, "git help" will open man pages only for actual commands.

Let's hide this option from command help of "git help" itself, drop
the short-and-sweet "-e", not command-line complete it, and leave it
not-mentioned here.

It is a different story if this option would really help the end
users, but I do not think that is the case.  If this were to face
the end users properly, we would need to worry about making sure
that "git help -g -e" would error out, and getting all the other
possible corner cases right.  I do not think the amount of effort
required to do so (even the "trying to enumerate what other possible
corner cases there may be" part) is worth it.

> In the test script we do two things I'd like to point out:
>
>> +   test_config help.htmlpath test://html &&
>
> As we pass a URL, Git won't check if the given path looks like
> a documentation directory.  Another solution would be to create
> a directory, add a file "git.html" to it and just use this path.

I think this is OK; with s|As we pass a URL|As we pass a string with
:// in it|, the first sentence can be a in-code comment in the test
that does this and will help readers of the code in the future.

>> +   test_config help.browser firefox
>
> Git checks if the browser is known, so the "test-browser" needs to
> pretend it is one of them.

Are you talking about the hardcoded list in valid_tool() helper
function in git-web--browse.sh?  If we use the established escape
hatch implemented by valid_custom_tool() helper there by setting
browser.*.cmd, would that be sufficient to work around the "Git
checks if the browser is known"?

> diff --git a/Documentation/git-help.txt b/Documentation/git-help.txt
> index 40d328a..eeb1950 100644
> --- a/Documentation/git-help.txt
> +++ b/Documentation/git-help.txt
> @@ -8,7 +8,7 @@ git-help - Display help information about Git
>  SYNOPSIS
>  
>  [verse]
> -'git help' [-a|--all] [-g|--guide]
> +'git help' [-a|--all] [-e|--exclude-guides] [-g|--guide]
>  [-i|--info|-m|--man|-w|--web] [COMMAND|GUIDE]

So, let's not do this.

> diff --git a/builtin/help.c b/builtin/help.c
> index e8f79d7..40901a9 100644
> --- a/builtin/help.c
> +++ b/builtin/help.c
> @@ -37,8 +37,10 @@ static int show_all = 0;
>  static int show_guides = 0;
>  static unsigned int colopts;
>  static enum help_format help_format = HELP_FORMAT_NONE;
> +static int exclude_guides;
>  static struct option builtin_help_options[] = {
>   OPT_BOOL('a', "all", _all, N_("print all available commands")),
> + OPT_BOOL('e', "exclude-guides", _guides, N_("exclude guides")),

So I'd suggest using PARSE_OPT_HIDDEN for this one and drop 'e' shorthand.
The only caller of this mode does not use it.

> diff --git a/contrib/completion/git-completion.bash 
> b/contrib/completion/git-completion.bash
> index c1b2135..b148164 100644
> --- a/contrib/completion/git-completion.bash
> +++ b/contrib/completion/git-completion.bash
> @@ -1393,7 +1393,7 @@ _git_help ()
>  {
>   case "$cur" in
>   --*)
> - __gitcomp "--all --guides --info --man --web"
> + __gitcomp "--all --exclude-guides --guides --info --man --web"
>   return
>   ;;
>   esac

So, let's not do this.

> diff --git a/t/t0012-help.sh b/t/t0012-help.sh
> new file mode 100755
> index 000..fb1abd7
> --- /dev/null
> +++ b/t/t0012-help.sh
> @@ -0,0 +1,33 @@
> +#!/bin/sh
> +
> +test_description='help'
> +
> +. ./test-lib.sh
> +
> +configure_help () {
> + test_config help.format html &&
> + test_config help.htmlpath test://html &&
> + test_config help.browser firefox 
> +}

Would replacing the last line with:

test_config browser.test.cmd ./test-browser &&
test_config help.browser test

and then writing to test-browser work just as well?  If so, that
would be much cleaner and more preferrable.

> +
> +test_expect_success "setup" "
> + write_script firefox <<-\EOF
> + exit 0
> + EOF
> +"

Unless there is a good reason you MUST do so, avoid quoting the test
body with double quotes, as it invites mistakes [*1*].

Also, how about using something like:

write_script test-browser <<-\EOF
i=0
for arg
do
i=$(( $i + 1 ))
echo "$i: $arg"
done >test-browser.log
EOF

instead?  That way, you can ensure that "git help status" attempts
to call git-status.html with the expected path, not gitstatus.html
or status.html, or somesuch, immediately after running "git help
status" in the next test by inspecting test-browser.log ...

> +test_expect_success "works for commands and guides by default" "
> + configure_help &&
> + git help status &&

... right here.

The output from the test-browser does not have to be multi-line;
just doing

echo "$*"

might be sufficient.

> + git help 

Re: [PATCH v11 5/8] allow do_submodule_path to work even if submodule isn't checked out

[Keller, Jacob E]

On Fri, 2016-08-26 at 11:19 -0700, Junio C Hamano wrote:
> Jacob Keller  writes:
> 
> > 
> > Currently, do_submodule_path will attempt locating the .git
> > directory by
> > using read_gitfile on /.git. If this fails it just assumes
> > the
> > /.git is actually a git directory.
> > 
> > This is good because it allows for handling submodules which were
> > cloned
> > in a regular manner first before being added to the parent project.
> 
> s/parent project/superproject/;
> 

Yep.

> > 
> > Unfortunately this fails if the  is not actually checked out
> > any
> > longer, such as by removing the directory.
> > 
> > Fix this by checking if the directory we found is actually a
> > gitdir. In
> > the case it is not, attempt to lookup the submodule configuration
> > and
> > find the name of where it is stored in the .git/modules/ folder of
> > the
> > parent project.
> 
> As you consistently say "directory" in the earlier part of the log
> message,
> 
> s/folder of the parent project/directory of the superproject/;
> 
> is desired.
> 

Yep.

> > 
> > 
> > If we can't locate the submodule configuration this might occur
> > because
> 
> I added s/configuration/&,/ to make it a bit easier to read.
> 

Makes sense.

> > 
> > for example a submodule gitlink was added but the corresponding
> > .gitmodules file was not properly updated. A die() here would not
> > be
> > pleasant to the users of submodule diff formats, so instead, modify
> > do_submodule_path to return an error code. For
> > git_pathdup_submodule,
> > just return NULL when we fail to find a path. For
> > strbuf_git_path_submodule
> > propagate the error code to the caller.
> 
> Somehow I had to read the latter half of this paragraph twice,
> before I realized that the last two sentence talks about how these
> two functions exactly do "to return an error code".  Tentatively
> what I queued has:
> 
> ... so instead, modify do_submodule_path() to return an error
> code:
> 
>  - git_pathdup_submodule() returns NULL when we fail to find a
> path.
>  - strbuf_git_path_submodule() propagates the error code to the
>    caller.
> 
> instead, hoping that would be easier to understand.
> 

That's much better and more clear. Thanks!

> > 
> > -static void do_submodule_path(struct strbuf *buf, const char
> > *path,
> > -     const char *fmt, va_list args)
> > +/* Returns 0 on success, non-zero on failure. */
> 
> s/non-zero/negative/;
> 


True.

> > 
> > +#define SUBMODULE_PATH_ERR_NOT_CONFIGURED -1
> > +static int do_submodule_path(struct strbuf *buf, const char *path,
> > +    const char *fmt, va_list args)
> >  {
> 
> This 5/8 is the only changed one in the entire series from the
> previous round, I think.  I'll replace what was in 'pu' and wait for
> responses.
> 
> Thanks.

Yep, that's correct.

Regards,
Jake

Re: [PATCH 2/2] SubmittingPatches: hint at gitk's "Copy commit summary" command

[Junio C Hamano]

Beat Bolli  writes:

> @@ -124,7 +124,8 @@ archive, summarize the relevant points of the discussion.
>  If you want to reference a previous commit in the history of a stable
>  branch use the format "abbreviated sha1 (subject, date)". So for example
>  like this: "Commit f86a374 (pack-bitmap.c: fix a memleak, 2015-03-30)
> -noticed [...]".
> +noticed [...]". The "Copy commit summary" command of gitk generates this
> +format.

(continuing from my 1/2 review)  And if people agree that the format
gitk already uses is better, this text should probably read more
like:

If you want to reference a previous commit in the history of a
stable branch, use the format "abbreviated sha1 (subject, date)",
with the subject enclosed in a pair of double-quotes, like this:

Commit f86a374 ("pack-bitmap.c: fix a memleak", 2015-03-30)
noticed that ...

The "Copy commit summary" command of gitk can be used to obtain this
format.
--
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 v10 0/9] submodule inline diff format

[Keller, Jacob E]

On Fri, 2016-08-26 at 10:35 -0700, Stefan Beller wrote:
> > a) read_gitfile on /.git
> > b) if read_gitfile succeeds, use it's contents, otherwise use
> > /.git for next steps
> > c) check if the resulting file is a git directory, we're fine.. we
> > found a gitdir, so stop.
> > d) otherwise,  empty the buffer, then lookup submodules
> > e) when submodules lookup succeeds.. see if we found a name. If so,
> > use that.
> 
> When the submodules lookup succeeds, we can assert the name exists.
> There is currently only one way the lookup is populated, and that is
> lookup_or_create_by_name in submodule-config.c:182, which fills in
> the name all the time.

Yes, that was how I was trying to word it, and that's what I've done in
code.

> 
> > 
> > f) if we didn't just exit with an empty buffer.
> > 
> > That empty buffer *should* trigger  revision error codes since it
> > won't point to any valid path and it also triggers the regular
> > error
> > code in add_submodule_odb so it handles that with showing not
> > initizlied.
> > 
> > This method is less work then re-implementing a _gently() variant
> > for
> > all of these functions.
> > 
> > Stefan, does this make sense and seem reasonable?
> 
> Sounds reasonable to me.
> 
> Thanks for working on this!
> Stefan

Thanks for review!

Regards,
Jake

Re: [PATCH 1/2] gitk: align the commit summary format to the documentation

[Junio C Hamano]

Beat Bolli  writes:

> In 175d38c (SubmittingPatches: document how to reference previous commits,
> 2016-07-28) the format for referring to older commits was specified.
>
> Make the text generated by the "Copy commit summary" command match this
> format.

Hmph.  I didn't know gitk already had its own command to produce a
short string.  I actually think what it produces

> In 175d38c ("SubmittingPatches: document how to reference previous commits",
> 2016-07-28) the format for referring to older commits was specified.

is easier to read when pasted into a sentence than what the recent
update 175d38ca ("SubmittingPatches: document how to reference
previous commits", 2016-07-28) suggests to do, i.e.

> In 175d38c (SubmittingPatches: document how to reference previous commits,
> 2016-07-28) the format for referring to older commits was specified.

Heiko, Stefan, I think you two were involved in adding that new
paragraph.   What do you think?
--
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 v11 5/8] allow do_submodule_path to work even if submodule isn't checked out

[Junio C Hamano]

Jacob Keller  writes:

> Currently, do_submodule_path will attempt locating the .git directory by
> using read_gitfile on /.git. If this fails it just assumes the
> /.git is actually a git directory.
>
> This is good because it allows for handling submodules which were cloned
> in a regular manner first before being added to the parent project.

s/parent project/superproject/;

> Unfortunately this fails if the  is not actually checked out any
> longer, such as by removing the directory.
>
> Fix this by checking if the directory we found is actually a gitdir. In
> the case it is not, attempt to lookup the submodule configuration and
> find the name of where it is stored in the .git/modules/ folder of the
> parent project.

As you consistently say "directory" in the earlier part of the log
message,

s/folder of the parent project/directory of the superproject/;

is desired.

>
> If we can't locate the submodule configuration this might occur because

I added s/configuration/&,/ to make it a bit easier to read.

> for example a submodule gitlink was added but the corresponding
> .gitmodules file was not properly updated. A die() here would not be
> pleasant to the users of submodule diff formats, so instead, modify
> do_submodule_path to return an error code. For git_pathdup_submodule,
> just return NULL when we fail to find a path. For strbuf_git_path_submodule
> propagate the error code to the caller.

Somehow I had to read the latter half of this paragraph twice,
before I realized that the last two sentence talks about how these
two functions exactly do "to return an error code".  Tentatively
what I queued has:

... so instead, modify do_submodule_path() to return an error code:

 - git_pathdup_submodule() returns NULL when we fail to find a path.
 - strbuf_git_path_submodule() propagates the error code to the
   caller.

instead, hoping that would be easier to understand.

> -static void do_submodule_path(struct strbuf *buf, const char *path,
> -   const char *fmt, va_list args)
> +/* Returns 0 on success, non-zero on failure. */

s/non-zero/negative/;

> +#define SUBMODULE_PATH_ERR_NOT_CONFIGURED -1
> +static int do_submodule_path(struct strbuf *buf, const char *path,
> +  const char *fmt, va_list args)
>  {

This 5/8 is the only changed one in the entire series from the
previous round, I think.  I'll replace what was in 'pu' and wait for
responses.

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

[PATCH v2 0/3] help: make option --help open man pages only for Git commands

[Ralf Thielow]

Changes in v2 are:
- add a patch from Dscho to make config variable 'help.browser' work on Windows 
again
- rename option "--command-only" to "--exclude-guides" which is less ambiguous 
in 'help' context
- improve test script
- refactor usage of argv_array in handle_builtin

Johannes Schindelin (1):
  Revert "display HTML in default browser using Windows' shell API"

Ralf Thielow (2):
  help: introduce option --exclude-guides
  help: make option --help open man pages only for Git commands

 Documentation/git-help.txt | 11 ++---
 builtin/help.c | 37 ++
 compat/mingw.c | 42 --
 compat/mingw.h |  3 ---
 contrib/completion/git-completion.bash |  2 +-
 git.c  | 15 +++-
 t/t0012-help.sh| 41 +
 7 files changed, 87 insertions(+), 64 deletions(-)
 create mode 100755 t/t0012-help.sh

-- 
2.9.2.912.gd0c0e83

--
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 1/3] Revert "display HTML in default browser using Windows' shell API"

[Ralf Thielow]

From: Johannes Schindelin 

Since 4804aab (help (Windows): Display HTML in default browser using
Windows' shell API, 2008-07-13), Git for Windows used to call
`ShellExecute()` to launch the default Windows handler for `.html`
files.

The idea was to avoid going through a shell script, for performance
reasons.

However, this change ignores the `help.browser` config setting. Together
with browsing help not being a performance-critical operation, let's
just revert that patch.

Signed-off-by: Johannes Schindelin 
Signed-off-by: Ralf Thielow 
---
 builtin/help.c |  7 ---
 compat/mingw.c | 42 --
 compat/mingw.h |  3 ---
 3 files changed, 52 deletions(-)

diff --git a/builtin/help.c b/builtin/help.c
index 8848013..e8f79d7 100644
--- a/builtin/help.c
+++ b/builtin/help.c
@@ -379,17 +379,10 @@ static void get_html_page_path(struct strbuf *page_path, 
const char *page)
free(to_free);
 }
 
-/*
- * If open_html is not defined in a platform-specific way (see for
- * example compat/mingw.h), we use the script web--browse to display
- * HTML.
- */
-#ifndef open_html
 static void open_html(const char *path)
 {
execl_git_cmd("web--browse", "-c", "help.browser", path, (char *)NULL);
 }
-#endif
 
 static void show_html_page(const char *git_cmd)
 {
diff --git a/compat/mingw.c b/compat/mingw.c
index 2b5467d..3fbfda5 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -1930,48 +1930,6 @@ int mingw_raise(int sig)
}
 }
 
-
-static const char *make_backslash_path(const char *path)
-{
-   static char buf[PATH_MAX + 1];
-   char *c;
-
-   if (strlcpy(buf, path, PATH_MAX) >= PATH_MAX)
-   die("Too long path: %.*s", 60, path);
-
-   for (c = buf; *c; c++) {
-   if (*c == '/')
-   *c = '\\';
-   }
-   return buf;
-}
-
-void mingw_open_html(const char *unixpath)
-{
-   const char *htmlpath = make_backslash_path(unixpath);
-   typedef HINSTANCE (WINAPI *T)(HWND, const char *,
-   const char *, const char *, const char *, INT);
-   T ShellExecute;
-   HMODULE shell32;
-   int r;
-
-   shell32 = LoadLibrary("shell32.dll");
-   if (!shell32)
-   die("cannot load shell32.dll");
-   ShellExecute = (T)GetProcAddress(shell32, "ShellExecuteA");
-   if (!ShellExecute)
-   die("cannot run browser");
-
-   printf("Launching default browser to display HTML ...\n");
-   r = HCAST(int, ShellExecute(NULL, "open", htmlpath,
-   NULL, "\\", SW_SHOWNORMAL));
-   FreeLibrary(shell32);
-   /* see the MSDN documentation referring to the result codes here */
-   if (r <= 32) {
-   die("failed to launch browser for %.*s", MAX_PATH, unixpath);
-   }
-}
-
 int link(const char *oldpath, const char *newpath)
 {
typedef BOOL (WINAPI *T)(LPCWSTR, LPCWSTR, LPSECURITY_ATTRIBUTES);
diff --git a/compat/mingw.h b/compat/mingw.h
index 95e128f..2cadb81 100644
--- a/compat/mingw.h
+++ b/compat/mingw.h
@@ -417,9 +417,6 @@ int mingw_offset_1st_component(const char *path);
 #include 
 #endif
 
-void mingw_open_html(const char *path);
-#define open_html mingw_open_html
-
 /**
  * Converts UTF-8 encoded string to UTF-16LE.
  *
-- 
2.9.2.912.gd0c0e83

--
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/3] help: make option --help open man pages only for Git commands

[Ralf Thielow]

If option --help is passed to a Git command, we try to open
the man page of that command.  However, we do it for both commands
and concepts.  Make sure it is an actual command.

This makes "git  --help" not working anymore, while
"git help " still works.

Signed-off-by: Ralf Thielow 
---
 Documentation/git-help.txt |  5 +++--
 git.c  | 15 ++-
 t/t0012-help.sh|  8 
 3 files changed, 25 insertions(+), 3 deletions(-)

diff --git a/Documentation/git-help.txt b/Documentation/git-help.txt
index eeb1950..8d21e9f 100644
--- a/Documentation/git-help.txt
+++ b/Documentation/git-help.txt
@@ -29,8 +29,9 @@ guide is brought up. The 'man' program is used by default for 
this
 purpose, but this can be overridden by other options or configuration
 variables.
 
-Note that `git --help ...` is identical to `git help ...` because the
-former is internally converted into the latter.
+Note that `git --help ...` is almost identical to `git help ...` because
+the former is internally converted into the latter with option --exclude-guides
+being added.
 
 To display the linkgit:git[1] man page, use `git help git`.
 
diff --git a/git.c b/git.c
index 0f1937f..1c61151 100644
--- a/git.c
+++ b/git.c
@@ -522,21 +522,34 @@ static void strip_extension(const char **argv)
 
 static void handle_builtin(int argc, const char **argv)
 {
+   struct argv_array args = ARGV_ARRAY_INIT;
const char *cmd;
struct cmd_struct *builtin;
 
strip_extension(argv);
cmd = argv[0];
 
-   /* Turn "git cmd --help" into "git help cmd" */
+   /* Turn "git cmd --help" into "git help --exclude-guides cmd" */
if (argc > 1 && !strcmp(argv[1], "--help")) {
+   int i;
+
argv[1] = argv[0];
argv[0] = cmd = "help";
+
+   for (i = 0; i < argc; i++) {
+   argv_array_push(, argv[i]);
+   if (!i)
+   argv_array_push(, "--exclude-guides");
+   }
+
+   argc++;
+   argv = args.argv;
}
 
builtin = get_builtin(cmd);
if (builtin)
exit(run_builtin(builtin, argc, argv));
+   argv_array_clear();
 }
 
 static void execv_dashed_external(const char **argv)
diff --git a/t/t0012-help.sh b/t/t0012-help.sh
index fb1abd7..2b90947 100755
--- a/t/t0012-help.sh
+++ b/t/t0012-help.sh
@@ -30,4 +30,12 @@ test_expect_success "--exclude-guides does not work for 
guides" "
test_i18ncmp expected actual
 "
 
+test_expect_success "--help does not work for guides" "
+   cat <<-EOF >expected &&
+   git: 'revisions' is not a git command. See 'git --help'.
+   EOF
+   test_must_fail git revisions --help 2>actual &&
+   test_i18ncmp expected actual
+"
+
 test_done
-- 
2.9.2.912.gd0c0e83

--
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 2/3] help: introduce option --exclude-guides

[Ralf Thielow]

Introduce option --exclude-guides to the help command.  With this option
being passed, "git help" will open man pages only for actual commands.

Since we know it is a command, we can use function help_unknown_command
to give the user advice on typos.

Helped-by: Johannes Schindelin 
Signed-off-by: Ralf Thielow 
---
In the test script we do two things I'd like to point out:

> +   test_config help.htmlpath test://html &&

As we pass a URL, Git won't check if the given path looks like
a documentation directory.  Another solution would be to create
a directory, add a file "git.html" to it and just use this path.

> +   test_config help.browser firefox

Git checks if the browser is known, so the "test-browser" needs to
pretend it is one of them.

 Documentation/git-help.txt |  6 +-
 builtin/help.c | 30 +++---
 contrib/completion/git-completion.bash |  2 +-
 t/t0012-help.sh| 33 +
 4 files changed, 62 insertions(+), 9 deletions(-)
 create mode 100755 t/t0012-help.sh

diff --git a/Documentation/git-help.txt b/Documentation/git-help.txt
index 40d328a..eeb1950 100644
--- a/Documentation/git-help.txt
+++ b/Documentation/git-help.txt
@@ -8,7 +8,7 @@ git-help - Display help information about Git
 SYNOPSIS
 
 [verse]
-'git help' [-a|--all] [-g|--guide]
+'git help' [-a|--all] [-e|--exclude-guides] [-g|--guide]
   [-i|--info|-m|--man|-w|--web] [COMMAND|GUIDE]
 
 DESCRIPTION
@@ -43,6 +43,10 @@ OPTIONS
Prints all the available commands on the standard output. This
option overrides any given command or guide name.
 
+-e::
+--exclude-guides::
+   Do not show help for guides.
+
 -g::
 --guides::
Prints a list of useful guides on the standard output. This
diff --git a/builtin/help.c b/builtin/help.c
index e8f79d7..40901a9 100644
--- a/builtin/help.c
+++ b/builtin/help.c
@@ -37,8 +37,10 @@ static int show_all = 0;
 static int show_guides = 0;
 static unsigned int colopts;
 static enum help_format help_format = HELP_FORMAT_NONE;
+static int exclude_guides;
 static struct option builtin_help_options[] = {
OPT_BOOL('a', "all", _all, N_("print all available commands")),
+   OPT_BOOL('e', "exclude-guides", _guides, N_("exclude guides")),
OPT_BOOL('g', "guides", _guides, N_("print list of useful 
guides")),
OPT_SET_INT('m', "man", _format, N_("show man page"), 
HELP_FORMAT_MAN),
OPT_SET_INT('w', "web", _format, N_("show manual in web browser"),
@@ -426,10 +428,29 @@ static void list_common_guides_help(void)
putchar('\n');
 }
 
+static const char *check_git_cmd(const char* cmd)
+{
+   char *alias;
+
+   if (is_git_command(cmd))
+   return cmd;
+
+   alias = alias_lookup(cmd);
+   if (alias) {
+   printf_ln(_("`git %s' is aliased to `%s'"), cmd, alias);
+   free(alias);
+   exit(0);
+   }
+
+   if (exclude_guides)
+   return help_unknown_cmd(cmd);
+
+   return cmd;
+}
+
 int cmd_help(int argc, const char **argv, const char *prefix)
 {
int nongit;
-   char *alias;
enum help_format parsed_help_format;
 
argc = parse_options(argc, argv, prefix, builtin_help_options,
@@ -469,12 +490,7 @@ int cmd_help(int argc, const char **argv, const char 
*prefix)
if (help_format == HELP_FORMAT_NONE)
help_format = parse_help_format(DEFAULT_HELP_FORMAT);
 
-   alias = alias_lookup(argv[0]);
-   if (alias && !is_git_command(argv[0])) {
-   printf_ln(_("`git %s' is aliased to `%s'"), argv[0], alias);
-   free(alias);
-   return 0;
-   }
+   argv[0] = check_git_cmd(argv[0]);
 
switch (help_format) {
case HELP_FORMAT_NONE:
diff --git a/contrib/completion/git-completion.bash 
b/contrib/completion/git-completion.bash
index c1b2135..b148164 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -1393,7 +1393,7 @@ _git_help ()
 {
case "$cur" in
--*)
-   __gitcomp "--all --guides --info --man --web"
+   __gitcomp "--all --exclude-guides --guides --info --man --web"
return
;;
esac
diff --git a/t/t0012-help.sh b/t/t0012-help.sh
new file mode 100755
index 000..fb1abd7
--- /dev/null
+++ b/t/t0012-help.sh
@@ -0,0 +1,33 @@
+#!/bin/sh
+
+test_description='help'
+
+. ./test-lib.sh
+
+configure_help () {
+   test_config help.format html &&
+   test_config help.htmlpath test://html &&
+   test_config help.browser firefox 
+}
+
+test_expect_success "setup" "
+   write_script firefox <<-\EOF
+   exit 0
+   EOF
+"
+
+test_expect_success "works for commands and guides by default" "
+   configure_help &&
+   git help status &&
+   git help revisions

Re: [PATCH 12/15] sequencer: lib'ify save_opts()

[Junio C Hamano]

Johannes Schindelin  writes:

>  static int pick_commits(struct commit_list *todo_list, struct replay_opts 
> *opts)
> @@ -1128,9 +1130,9 @@ int sequencer_pick_revisions(struct replay_opts *opts)
>   return -1;
>   if (get_sha1("HEAD", sha1) && (opts->action == REPLAY_REVERT))
>   return error(_("Can't revert as initial commit"));
> - if (save_head(sha1_to_hex(sha1)))
> + if (save_head(sha1_to_hex(sha1)) ||
> + save_opts(opts))
>   return -1;
> - save_opts(opts);

I think it is much easier to read to keep this on a single line.  It
would be more verbose but an even easier would be to keep these two
operations two separate steps, i.e.

if (save_head())
return -1;
if (save_opts())
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: [PATCH 07/15] sequencer: lib'ify read_populate_opts()

[Junio C Hamano]

Johannes Schindelin  writes:

> -static void read_populate_opts(struct replay_opts **opts_ptr)
> +static int read_populate_opts(struct replay_opts **opts)
>  {
>   if (!file_exists(git_path_opts_file()))
> - return;
> - if (git_config_from_file(populate_opts_cb, git_path_opts_file(), 
> *opts_ptr) < 0)
> - die(_("Malformed options sheet: %s"), git_path_opts_file());
> + return 0;
> + if (git_config_from_file(populate_opts_cb, git_path_opts_file(), *opts) 
> < 0)
> + return error(_("Malformed options sheet: %s"),
> + git_path_opts_file());
> + return 0;

This may not be sufficient to avoid die(), unless we know that the
file we are reading is syntactically sound.  git_config_from_file()
will die in config.c::git_parse_source() when the config_source sets
die_on_error, and it is set in config.c::do_config_from_file().

The source we are reading from is created when the sequencer
machinery starts and is only written by save_opts() which is
called by the sequencer machinery using git_config_set*() calls,
so I think it is OK to assume that we won't hit errors that would
cause git_config_from_file() to die, at least for now.


--
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 v10 0/9] submodule inline diff format

[Stefan Beller]

On Thu, Aug 25, 2016 at 3:46 PM, Jacob Keller  wrote:
> On Thu, Aug 25, 2016 at 3:38 PM, Junio C Hamano  wrote:
>> Jacob Keller  writes:
>>
>>> So we should support the gitlink to a repository stored at 
>>> without stuff inside the .git/modules, and we should support submodule
>>> gitlinks with a proper .gitmodules setup. I don't think we should
>>> die() but we should error properly so I will introduce a _gently()
>>> variant of these functions, and die properly in the regular flow.
>>
>> Because "git diff [--cached] []" in the top-level is
>> driven by a gitlink in the index, immediately after adding a new
>> submodule to the index but before describing it in .gitmodules you
>> might not have a name (and you know in that case the path will
>> become the name when adding it to .gitmodules).  Also a gitlink in
>> the index may correspond to a submodule the user of the top-level is
>> not interested in, so there may not be anything in .git/modules/
>> that corresponds to it.  In these cases, I suspect that you do not
>> want to die, but you can just tell the user "I do not have enough
>> information to tell you a useful story yet".
>>
>
> Right. submodule_from_path() fails to find a config. I don't think
> die() is right here, because there is no easy way to make this into a
> gently() variant I can still do it if we think a die() is
> worthwhile otherwise for the other callers of do_submodule_path...
>
> However, I think the safest thing is to just:
>
> a) read_gitfile on /.git
> b) if read_gitfile succeeds, use it's contents, otherwise use
> /.git for next steps
> c) check if the resulting file is a git directory, we're fine.. we
> found a gitdir, so stop.
> d) otherwise,  empty the buffer, then lookup submodules
> e) when submodules lookup succeeds.. see if we found a name. If so, use that.

When the submodules lookup succeeds, we can assert the name exists.
There is currently only one way the lookup is populated, and that is
lookup_or_create_by_name in submodule-config.c:182, which fills in
the name all the time.

> f) if we didn't just exit with an empty buffer.
>
> That empty buffer *should* trigger  revision error codes since it
> won't point to any valid path and it also triggers the regular error
> code in add_submodule_odb so it handles that with showing not
> initizlied.
>
> This method is less work then re-implementing a _gently() variant for
> all of these functions.
>
> Stefan, does this make sense and seem reasonable?

Sounds reasonable to me.

Thanks for working on this!
Stefan
--
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 v6 03/13] pkt-line: add packet_write_fmt_gently()

[Jeff King]

On Fri, Aug 26, 2016 at 10:10:50AM -0700, Junio C Hamano wrote:

> Lars Schneider  writes:
> 
> > I agree with your criticism of the code duplication. 
> >
> > However, I thought it would be OK, as Peff already 
> > tried to refactor it...
> > http://public-inbox.org/git/20160810150139.lpxyrqkr53s5f...@sigill.intra.peff.net/
> >
> > ... and I got the impression you agreed with Peff:
> > http://public-inbox.org/git/xmqqvaz84g9y@gitster.mtv.corp.google.com/
> 
> The former does not exactly show how ugly it was, but I do not have
> to see it.  It is talking about eliminating the need for memcpy()
> and duplicated header generation code, which the suggestion you are
> responding to didn't even attempt.  If Peff said he tried an even
> more aggressive refactoring and it ended up too ugly to live, I
> believe him and agree with his assessment.

Right, what I found difficulty with was factoring out format_packet(). I
think the "gently" part is easy.

-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: [PATCH v6 06/13] pkt-line: add functions to read/write flush terminated packet streams

[Jeff King]

On Fri, Aug 26, 2016 at 10:02:52AM -0700, Stefan Beller wrote:

> Yeah. To me it seems this design explicitly makes it hard for side bands.
> As we do not need sidebands for local transfers, this is fine for sure.
> 
> (If we wanted to make it sideband friendly, I'd expect you could register
> callbacks for either all packets or for a given sideband until the next
> flush comes.)
> 
> So as hinted by this design, we want a protocol that
> * doesn't care about sidebands
> * cares about large data (hence maybe throughput)
> * has easy/clean interface
> 
> And one large packet would suffice for these three points as well
> and additionally has benefits for the network stuff.
> 
> The 320kB additional transmission are negligible overhead, so I was not
> concerned about the size, but rather the code being bloated, i.e. we need
> one layer of additional code to cope with the repetitive packets.

Maybe I don't understand what you mean by "one large packet". But if you
mean sending "I am about to send you 100MB" followed by the 100MB, the
point is that the sender does not necessarily have that exact value
ahead of time. So it would want to write it in chunks.

E.g., consider a clean which replaces s/foo/bar/ in its content. It
would write out all the content up to the next "foo", then write "bar",
and repeat.

We could let each chunk be arbitrarily big. I.e., "I am about to send
you 50MB", then "here are 3 bytes", then "here are the other 50MB".

But using a fixed-size length header makes the packets easy to generate
and parse. And 64KB is small enough that senders and receivers can
easily buffer single packets, making interfaces simpler. And it's big
enough that the overhead (4 bytes per 64KB) is negligible.

Anyway. It certainly does not seem worth moving the network protocols to
a new data format. The compatibility changes would not be worth it. But
if we _were_ to do so, there are tons of efficient well-thought-out
data-marshaling formats we could use rather than inventing our own. But
I just don't see the benefit.

> ---
> My background is mostly submodule related, and whenever I come up
> with a shiny novel idea that would help submodules tremendously, someone
> (mostly Peff) comes along and suggests a broader more generic thing, that
> works just as well for submodules but is applicable to all of Git.

Heh. It is not always a good thing, if it derails the original purpose
of the code (and I include some of my suggestions in that). :)

The trick is to make it general enough, without getting lost in the
weeds of what _might_ happen in the future and wasting time (and
adding complexity).

-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: [PATCH v6 06/13] pkt-line: add functions to read/write flush terminated packet streams

[Junio C Hamano]

Lars Schneider  writes:

> OK, what function names would be more clear from your point of view?
>
> write_packetized_stream_from_fd()
> write_packetized_stream_from_buf()
> read_packetized_stream_to_buf()

Would

write_packetized_from_fd()
write_packetized_from_buf()
read_packetized_to_buf()

be shorter, still understandable, be consistent with the existing
convention to name write_/read_ a function that shuffles bytes via a
file descriptor, and to the point, perhaps?

--
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 v6 06/13] pkt-line: add functions to read/write flush terminated packet streams

[Junio C Hamano]

Jacob Keller  writes:

> Stefan's argument to me is thus "If we're already going to ignore
> sideband packets here, why not go all the way and make variable length
> packets and send a single packet of a maximum length? Doing thus will
> solve some set of future problems nicely and makes this code easier."
>
> I'm not sure I agree myself, but that's the logic as I understand it.

I do not know if I agree, but now I understand the flow of logic.

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 v6 05/13] pkt-line: add packet_write_gently()

[Junio C Hamano]

Lars Schneider  writes:

>> Do you anticipate future need of non-gently variant of this
>> function?  If so, perhaps a helper that takes a boolean "am I
>> working for the gently variant?" may help share more code.
>
> With helper you mean "an additional boolean parameter"? I don't 
> see a need for a non-gently variant right now but I will
> add this parameter if you think it is a good idea. How would the
> signature look like?
>
> int packet_write_gently(const int fd_out, const char *buf, size_t size, int 
> gentle)
>
> This would follow type_from_string_gently() in object.h.

I actually imagined it would be more like your packet_write_fmt vs
packet_write_fmt_gently pair of functions.  If you do not have an
immediate need for a non-gentle packet_write() right now, but you
still forsee that it is likely some other caller may want one, you
could still prepare for it by doing a static

packet_write_1((const int fd_out, const char *buf, size_t size, int 
gentle)

and make packet_write_gently() call it with gentle=1, without
actually introducing packet_write() nobody yet calls.


--
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 v6 03/13] pkt-line: add packet_write_fmt_gently()

[Junio C Hamano]

Lars Schneider  writes:

> I agree with your criticism of the code duplication. 
>
> However, I thought it would be OK, as Peff already 
> tried to refactor it...
> http://public-inbox.org/git/20160810150139.lpxyrqkr53s5f...@sigill.intra.peff.net/
>
> ... and I got the impression you agreed with Peff:
> http://public-inbox.org/git/xmqqvaz84g9y@gitster.mtv.corp.google.com/

The former does not exactly show how ugly it was, but I do not have
to see it.  It is talking about eliminating the need for memcpy()
and duplicated header generation code, which the suggestion you are
responding to didn't even attempt.  If Peff said he tried an even
more aggressive refactoring and it ended up too ugly to live, I
believe him and agree with his assessment.

> I will try to refactor it according to your suggestion above. 
> Would "packet_write_fmt_1()" be an acceptable name or should 
> I come up with something more expressive?

The latter is preferrable, but we do not mind too strongly about
the name of file-scope static helper that will never be called
directly by anybody other than the two more public entry points the
helper was designed to serve.
--
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] gitk: align the commit summary format to the documentation

[Beat Bolli]

In 175d38c (SubmittingPatches: document how to reference previous commits,
2016-07-28) the format for referring to older commits was specified.

Make the text generated by the "Copy commit summary" command match this
format.

Signed-off-by: Beat Bolli 
Cc: Paul Mackerras 
---
 gitk-git/gitk | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/gitk-git/gitk b/gitk-git/gitk
index 805a1c7..a27bf99 100755
--- a/gitk-git/gitk
+++ b/gitk-git/gitk
@@ -9382,7 +9382,7 @@ proc mktaggo {} {
 proc copysummary {} {
 global rowmenuid autosellen
 
-set format "%h (\"%s\", %ad)"
+set format "%h (%s, %ad)"
 set cmd [list git show -s --pretty=format:$format --date=short]
 if {$autosellen < 40} {
 lappend cmd --abbrev=$autosellen
-- 
2.7.2
--
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] SubmittingPatches: hint at gitk's "Copy commit summary" command

[Beat Bolli]

Amend the section on referencing previous commits with a hint to the
gitk command that was added exactly for this purpose.

Signed-off-by: Beat Bolli 
---
 Documentation/SubmittingPatches | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/Documentation/SubmittingPatches b/Documentation/SubmittingPatches
index 500230c..94a1661 100644
--- a/Documentation/SubmittingPatches
+++ b/Documentation/SubmittingPatches
@@ -124,7 +124,8 @@ archive, summarize the relevant points of the discussion.
 If you want to reference a previous commit in the history of a stable
 branch use the format "abbreviated sha1 (subject, date)". So for example
 like this: "Commit f86a374 (pack-bitmap.c: fix a memleak, 2015-03-30)
-noticed [...]".
+noticed [...]". The "Copy commit summary" command of gitk generates this
+format.
 
 
 (3) Generate your patch using Git tools out of your commits.
-- 
2.7.2
--
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 v6 06/13] pkt-line: add functions to read/write flush terminated packet streams

[Stefan Beller]

On Thu, Aug 25, 2016 at 5:55 PM, Jacob Keller  wrote:
> On Thu, Aug 25, 2016 at 3:31 PM, Junio C Hamano  wrote:
>> What is wrong about that?  4*80k = 320kB overhead for length fields
>> to transfer 5GB worth of data?  I do not think it is worth worrying
>> about it.
>>
>> But I am more surprised by seeing that "why not a single huge
>> packet" suggestion immediately after you talked about "without the
>> possibility to intervene".  They do not seem to be remotely related;
>> in fact, they are going into opposite directions.
>>
>> Puzzled.
>
> Stefan's argument to me is thus "If we're already going to ignore
> sideband packets here, why not go all the way and make variable length
> packets and send a single packet of a maximum length? Doing thus will
> solve some set of future problems nicely and makes this code easier."
>
> I'm not sure I agree myself, but that's the logic as I understand it.

Yeah. To me it seems this design explicitly makes it hard for side bands.
As we do not need sidebands for local transfers, this is fine for sure.

(If we wanted to make it sideband friendly, I'd expect you could register
callbacks for either all packets or for a given sideband until the next
flush comes.)

So as hinted by this design, we want a protocol that
* doesn't care about sidebands
* cares about large data (hence maybe throughput)
* has easy/clean interface

And one large packet would suffice for these three points as well
and additionally has benefits for the network stuff.

The 320kB additional transmission are negligible overhead, so I was not
concerned about the size, but rather the code being bloated, i.e. we need
one layer of additional code to cope with the repetitive packets.

---
My background is mostly submodule related, and whenever I come up
with a shiny novel idea that would help submodules tremendously, someone
(mostly Peff) comes along and suggests a broader more generic thing, that
works just as well for submodules but is applicable to all of Git.

So I picked up that way of thinking: If we write code here, that helps with a
very special niche of things, can we make so, that the rest also benefits?

I may not understand all the requirements in this case, but to me it looks like
the "one packet" approach covers all requirements, but has the huge potential
to make other parts better in the long run.
-- 

Thanks,
Stefan
--
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 06/15] sequencer: lib'ify read_populate_todo()

[Junio C Hamano]

Johannes Schindelin  writes:

> In short: I would really appreciate it if you could cut quoted text after
> your last response.

I think you are referring to the patch part in this case.

As I was not making point-by-point comments on the proposed commit
log message, quoting only that part and cutting the patch text would
have been pointless.  I could have cut the proposed log message and
left the patch in, though, because the comments were not about what
was in the proposed log message, but about what was not in it [*1*].

I left the patch part for other people's use, to make it easy for
them to see what I was saying was correct and appropriate for what
the patch does.  Removing that would not have made much sense.

But that is only true in this case.  I try to see if I can trim
quote more aggressively, but I would still err on the side of
over-quoting than under-quoting [*2*].


[Footnote]

*1* As to what was IN the proposed log message, I have one comment.
I do not think "To be truly useful" adds any value, as there is
nothing "truly" about what this series does.  The original was
"truly" useful for the purpose of the sequencer machinery and its
use in the current callers, just like with this series it becomes
"truly" useful for envisioned new callers that want to handle error
conditions themselves.  The change is making it "more useful" for
one different use case.  It may not be "truly" useful for other use
cases that sequencer machinery could be used even with the
"eradicate die and exit" change and other people may bring a new use
case that wants it to be even "more useful".  The fact that it may
not be directly usable by a new use case without further change does
not make the result of applying this proposed series less than
"truly useful".  The "truly" adjective implies absolute, but there
is nothing absolute in incremental improvements.  It is always
relative to the context within which the machinery was designed to
be used.  "To make it usable for callers that want to handle errors
themselves (instead of just dying and the calling process handle
it), let's turn die's and exit's to returning negative values." is
probably closer to what I would have expected.

*2* As I read the quoted part before sending my response out, erring
on the side not to underquote tends to avoid a mistake that invites:
"I think you misunderstood what I wrote in the part you snipped from
your quote; perhaps you skimmed it without fully reading."
--
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 v14 07/27] bisect--helper: `bisect_reset` shell function in C

[Junio C Hamano]

Pranit Bauva  writes:

>> Also this version fails to catch "bisect reset a b c" as an error, I
>> suspect.
>
> It didn't when I tried it right now. Could you please elaborate on why
> you think it can fail? There might be a thing which I haven't tested.

My bad.  I just compared your bisect_reset() implementation that had

if (no specific commit) {
reset to the branch
} else {
reset to the commit
}

with the original

case $# in
0)  reset to the branch ;;
1)  reset to the commit ;;
*)  give usage and die ;;
esac

and took the difference and reacted "ah, excess parameters are not
diagnosed in this function".

Your caller does complain about excess parameters without giving
usage, and that is what I missed.

I am not sure if you intended to change the behaviour in this case
to avoid giving the usage string; I tend to think it is a good
change, but I didn't see it mentioned in the proposed commit log,
which also contributed to my not noticing the test in the caller.

--
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 v6 06/12] doc: revisions: single vs multi-parent notation comparison

[Junio C Hamano]

Jakub Narębski  writes:

> W dniu 13.08.2016 o 01:45, Philip Oakley pisze:
>
>> +'HEAD{caret}2{caret}@', however you cannot say 'HEAD{caret}@{caret}2'.
>
> Though I do wonder if it is implementation limitation, or if it is something
> inherent in the notation, namely that ^@ and ^! resolve (the former
> at least in general, though not for every ) to more than one revision
> specifier.

I think that the answer you are giving to the question is the latter,
and I tend to agree with you.

You could define: A^@ followed by  means first expand A^@,
and then apply that  to each and every one in the resulting
set.  But some may be merges and while others not.  It is unclear
what should happen to A^@^2 when one of A's parents has only one
parent.  In some use cases, it would be convenient if parents of A
to which the  operation cannot be applied, but in other
use cases, it would be convenient if the whole thing caused an error.
The notation is not detailed/rich enough to express the distinction.


--
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 v5 01/12] doc: use 'symmetric difference' consistently

[Junio C Hamano]

Jakub Narębski  writes:

>>  --left-right::
>> -Mark which side of a symmetric diff a commit is reachable from.
>> +Mark which side of a symmetric difference a commit is reachable from.
>>  Commits from the left side are prefixed with `<` and those from
>>  the right with `>`.  If combined with `--boundary`, those
>>  commits are prefixed with `-`.
>
> That's very nice that two^W three related options, namely --left-only,
> --right-only and --left-right now use the same notation.
>
> I guess that "symmetric range" was to mean "symmetric difference range".

By "symmetric difference", we mean a "set difference" operation that
is symmetric.  Contrasting with asymmetric difference that gives a
set of elements that belong to set A but do not belong to set B
(i.e. B..A), symmetric difference of set A and set B gives a set of
elements that belong to either set A alone or set B alone but not
both (i.e. A...B).

So if you really want a fully spelled name, we would not want to
call it with a name with "range".  It is "symmetric set difference",
and "symmetric difference" in the updated sentence is a short-hand
for that.
--
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's cooking in git.git (Aug 2016, #08; Wed, 24)

[Junio C Hamano]

Johannes Schindelin  writes:

> On Wed, 24 Aug 2016, Junio C Hamano wrote:
>
>> * rt/help-unknown (2016-08-18) 2 commits
>>  - help: make option --help open man pages only for Git commands
>>  - help: introduce option --command-only
>> 
>>  "git nosuchcommand --help" said "No manual entry for gitnosuchcommand",
>>  which was not intuitive, given that "git nosuchcommand" said "git:
>>  'nosuchcommand' is not a git command".
>> 
>>  Waiting for the review discussion to settle.
>
> Not only that. For a week now, two of my build jobs have been failing (the
> one testing `pu` verbatim and the one testing Git for Windows' patches
> rebased on top of `pu`) because the test does not actually test what was
> just built, but relies on the Git man pages to be installed.
>
> So it needs more than just the review discussion to settle.

Ah, sorry for being inaccurate.

A topic may not be ready to be merged down to the next integration
branch before being re-rolled.  I say "Waiting for a reroll" when I
saw review comments and responses have produced enough material to
go into a reroll and the ball is in the submitter's court.  While
the review is still ongoing, I just label it differently to as a
reminder to myself that it is premature for me to jump up and down
and demand "Can we have a reroll pretty-please now?"  This was in
that latter kind when I labelled it.

I think the reviews on the topic has settled and I should have
marked it as "Waiting for a reroll", but during the prerelease
period, I do not have time to pay too much attention on things
outside 'master' and fix-up topics that are moving through 'next'
to 'master' aiming to become part of the upcoming release, so the
marking was left behind [*1*].  My bad.


[Footnotes]

*1* It follows that it would not be unusual if the tip of 'pu' does
not even build during the time.  Being able to test it does not
contribute much to catching the remaining bug in the release
candidates and I'd really like to see all of us paying more
attention to the tip of 'master'.  A statement "With topic X
that is still in 'next', A works" is not all that interesting
unless it is accompanied by "And A used to work in the previous
release, but broken in 'master'." at this point in a development
cycle.


--
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 v6 06/12] doc: revisions: single vs multi-parent notation comparison

[Jakub Narębski]

W dniu 13.08.2016 o 01:45, Philip Oakley pisze:

> --- a/Documentation/revisions.txt
> +++ b/Documentation/revisions.txt
> @@ -284,6 +284,10 @@ The 'r1{caret}@' notation means all parents of 'r1'.
>  The 'r1{caret}!' notation includes commit 'r1' but excludes all of its 
> parents.
>  By itself, this notation denotes the single commit 'r1'.
>  
> +While '{caret}' was about specifying a single commit parent, these
> +two notations consider all its parents. For example you can say
> +'HEAD{caret}2{caret}@', however you cannot say 'HEAD{caret}@{caret}2'.
> +

That's good to have.

Though I do wonder if it is implementation limitation, or if it is something
inherent in the notation, namely that ^@ and ^! resolve (the former
at least in general, though not for every ) to more than one revision
specifier.

Anyway, good addition!
-- 
Jakub Narębski

--
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: Feature Request: Branch-Aware Submodules

[Hedges Alexander]

> On 25 Aug 2016, at 19:45, Stefan Beller  wrote:
> 
> +cc Jacob and Lars who work with submodules as well.
> 
> On Thu, Aug 25, 2016 at 2:00 AM, Hedges  Alexander
>  wrote:
>> 
>> Right now updating a submodule in a topic branch and merging it into master
>> will not change the submodule index in master leading to at least two commit
>> for the same change (one in any active branch). This happened to me quite a 
>> few
>> times. To a newcomer this behavior is confusing and it leads to unnecessary
>> commits.
> 
> So you roughly do
> 
>   git checkout -b new-topic
>   # change the submodule to point at the latest upstream version:
>   git submodule update --remote 
>   git commit -a -m "update submodule"
>   git checkout master
>   git merge new-topic
>   # here seems to be your point of critic?
>   # now the submodule pointer would still point to the latest
> upstream version?
> 

Excuse my poor wording above. The problem is the following:

# assume a repo with a few branches and one submodules
git checkout -b new_feature
git commit -am "some new commits"
cd submodule/path
git commit -am "dirty hacking on a library"
cd ../..
git commit -am "changes and update library"
git status
# all is well
git checkout master
git status
# it says new submodule commits ??
git commit -am "update library again…"
git merge new_feature
git checkout old_feature_that_never_made_it
git status
# still ???
git commit -am …

Now reading the comments below, I overlooked git submodule update. I used update
only the first time after a clone with the init flag. As a remedy I could just
run git submodule update after every merge, but then I always get a detached
head which is also not ideal.
The second thing I overlooked is just merging without worrying about the git
status telling me the repository is dirty. But here my muscle memory does a
commit when the repository is dirty, before running any other git commands.

Obviously, its confusing to people without a certain amount of experience.

>> The proposed change would be to have a submodule either ignored or tracked by
>> the .gitmodules file.
>> If it is ignored, as for instance after a clone of the superproject, git 
>> simply
>> ignores all files in the submodule directory. The content of the gitmodules
>> file is then also not updated by git.
>> If it is not ignored, the .gitmodules is updated every time a commit happens 
>> in
>> the submodule.
> 
> So
> 
>   git -C  commit
> 
> should trigger a commit in the superproject as well, that changes the 
> gitmodules
> file? What do you record in the git modules file that needs updating?
> As the version is tracked via the gitlink entry, I do not see the
> information that
> needs tracking here?

I guess nothing has to be done here. I mistakenly thought the .gitmodules stores
the SHA.

> 
>> On branch switches the revision shown in the gitmodules from
>> that branch is checked out.
> 
> So you are proposing to put the revision into the gitmodules file?
> That would be redundant with the actual gitlink entry in your tree.
> (as shown via `git submodule status`)
> What would happen if the recorded revision in the gitmodules file and the
> gitlink are out of sync?
> 
> Oh, are you just proposing to actually make `git checkout` aware of the
> submodules? See[1]. I would welcome such a change and be happy th
> 
> [1] https://github.com/jlehmann/git-submod-enhancements
> which has some attempts for checkout including the submodules.
> I also tried writing some patches which integrate checking out submodules
> via checkout as well. A quicker `solution` would be a config option that
> just runs `git submodule update` after each checkout/pull etc.
> 

I see. The quick fix is almost what I’m looking for, except that it leaves
the repo in a detached head state. Could the submodule update be made 
automatically and intelligently pick the branch?

> 
>> This change would have submodules conceptually behave more like files to the
>> superproject.
>> 
>> 
>> Like current behavior, git status would display whether the submodule has
>> uncommitted changes or is at a new commit.
> 
> See config options diff.submodule and status.submoduleSummary.
> 

I meant that git status works fine the way it is implemented right now.

> 
>> 
>> I couldn't find any discussions on the initial implementation of 
>> git-submodule
>> or any previous proposals related to this in nature due to gmane being down
>> right now and the mailing list archives on the other sites are not great for
>> searching. So please excuse me if I'm bringing up already discussed stuff.
> 
> https://public-inbox.org/git for reading on the web, or
> 
>   git clone https://public-inbox.org/git
> 
> for reading offline.
> 

Thanks.

Best Regards,
Alexander Hedges

Re: What's cooking in git.git (Aug 2016, #08; Wed, 24)

[Johannes Schindelin]

Hi Junio,

On Wed, 24 Aug 2016, Junio C Hamano wrote:

> * rt/help-unknown (2016-08-18) 2 commits
>  - help: make option --help open man pages only for Git commands
>  - help: introduce option --command-only
> 
>  "git nosuchcommand --help" said "No manual entry for gitnosuchcommand",
>  which was not intuitive, given that "git nosuchcommand" said "git:
>  'nosuchcommand' is not a git command".
> 
>  Waiting for the review discussion to settle.

Not only that. For a week now, two of my build jobs have been failing (the
one testing `pu` verbatim and the one testing Git for Windows' patches
rebased on top of `pu`) because the test does not actually test what was
just built, but relies on the Git man pages to be installed.

So it needs more than just the review discussion to settle.

Ciao,
Dscho
--
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 02/14] sequencer: do not die() in do_pick_commit()

[Johannes Schindelin]

Instead of dying there, let the caller high up in the callchain
notice the error and handle it (by dying, still).

The eventual caller of do_pick_commit() is sequencer_pick_revisions(),
which already relays a reported error from its helper functions
(including this one), and both of its two callers know how to react to
a negative return correctly.

So this makes do_pick_commit() callable from new callers that want it
not to die, without changing the external behaviour of anything
existing.

Signed-off-by: Johannes Schindelin 
---
 sequencer.c | 14 --
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index 76b1c52..baf6b40 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -585,12 +585,14 @@ static int do_pick_commit(struct commit *commit, struct 
replay_opts *opts)
 * However, if the merge did not even start, then we don't want to
 * write it at all.
 */
-   if (opts->action == REPLAY_PICK && !opts->no_commit && (res == 0 || res 
== 1))
-   update_ref(NULL, "CHERRY_PICK_HEAD", commit->object.oid.hash, 
NULL,
-  REF_NODEREF, UPDATE_REFS_DIE_ON_ERR);
-   if (opts->action == REPLAY_REVERT && ((opts->no_commit && res == 0) || 
res == 1))
-   update_ref(NULL, "REVERT_HEAD", commit->object.oid.hash, NULL,
-  REF_NODEREF, UPDATE_REFS_DIE_ON_ERR);
+   if (opts->action == REPLAY_PICK && !opts->no_commit && (res == 0 || res 
== 1) &&
+   update_ref(NULL, "CHERRY_PICK_HEAD", commit->object.oid.hash, NULL,
+  REF_NODEREF, UPDATE_REFS_MSG_ON_ERR))
+   res = -1;
+   if (opts->action == REPLAY_REVERT && ((opts->no_commit && res == 0) || 
res == 1) &&
+   update_ref(NULL, "REVERT_HEAD", commit->object.oid.hash, NULL,
+  REF_NODEREF, UPDATE_REFS_MSG_ON_ERR))
+   res = -1;
 
if (res) {
error(opts->action == REPLAY_REVERT
-- 
2.10.0.rc1.99.gcd66998


--
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 01/14] sequencer: lib'ify sequencer_pick_revisions()

[Johannes Schindelin]

Instead of dying there, let the caller high up in the callchain notice
the error and handle it (by dying, still).

The function sequencer_pick_revisions() has only two callers,
cmd_revert() and cmd_cherry_pick(), both of which check the return
value and react appropriately upon errors.

So this is a safe conversion to make sequencer_pick_revisions()
callable from new callers that want it not to die, without changing
the external behaviour of anything existing.

Signed-off-by: Johannes Schindelin 
---
 sequencer.c | 9 +
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index 3804fa9..76b1c52 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -1063,10 +1063,11 @@ int sequencer_pick_revisions(struct replay_opts *opts)
if (!get_sha1(name, sha1)) {
if (!lookup_commit_reference_gently(sha1, 1)) {
enum object_type type = sha1_object_info(sha1, 
NULL);
-   die(_("%s: can't cherry-pick a %s"), name, 
typename(type));
+   return error(_("%s: can't cherry-pick a %s"),
+   name, typename(type));
}
} else
-   die(_("%s: bad revision"), name);
+   return error(_("%s: bad revision"), name);
}
 
/*
@@ -1082,10 +1083,10 @@ int sequencer_pick_revisions(struct replay_opts *opts)
!opts->revs->cmdline.rev->flags) {
struct commit *cmit;
if (prepare_revision_walk(opts->revs))
-   die(_("revision walk setup failed"));
+   return error(_("revision walk setup failed"));
cmit = get_revision(opts->revs);
if (!cmit || get_revision(opts->revs))
-   die("BUG: expected exactly one commit from walk");
+   return error("BUG: expected exactly one commit from 
walk");
return single_pick(cmit, opts);
}
 
-- 
2.10.0.rc1.99.gcd66998


--
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 00/14] Lib'ify quite a few functions in sequencer.c

[Johannes Schindelin]

This patch series is one of the half dozen patch series left to move the
bulk of rebase -i into a builtin.

The purpose of this patch series is to switch the functions in
sequencer.c from die()ing to returning errors instead, as proper library
functions should do, to give callers a chance to clean up after an
error.

Changes since v1:

- two "return error()"s replacing "die_errno()"s were turned into "return
  error_errno()"s instead.

- an strbuf is now released when format_todo() failed (and may have left
  the strbuf with allocated memory).

- a superfluous space (which was inherited from the previous code) was
  fixed, while at it.

- fixed commit messages to report that callers of the libified functions
  are already libified.

- reordered patches to ensure that callers of libified functions are
  already libified.


Johannes Schindelin (14):
  sequencer: lib'ify sequencer_pick_revisions()
  sequencer: do not die() in do_pick_commit()
  sequencer: lib'ify write_message()
  sequencer: lib'ify do_recursive_merge()
  sequencer: lib'ify do_pick_commit()
  sequencer: lib'ify walk_revs_populate_todo()
  sequencer: lib'ify prepare_revs()
  sequencer: lib'ify read_and_refresh_cache()
  sequencer: lib'ify read_populate_todo()
  sequencer: lib'ify read_populate_opts()
  sequencer: lib'ify create_seq_dir()
  sequencer: lib'ify save_head()
  sequencer: lib'ify save_todo()
  sequencer: lib'ify save_opts()

 sequencer.c | 172 
 1 file changed, 104 insertions(+), 68 deletions(-)

Published-As: https://github.com/dscho/git/releases/tag/libify-sequencer-v2
Fetch-It-Via: git fetch https://github.com/dscho/git libify-sequencer-v2

Interdiff vs v1:

 diff --git a/sequencer.c b/sequencer.c
 index caba11d..b6481bb 100644
 --- a/sequencer.c
 +++ b/sequencer.c
 @@ -464,7 +464,7 @@ static int do_pick_commit(struct commit *commit, struct 
replay_opts *opts)
 * to work on.
 */
if (write_cache_as_tree(head, 0, NULL))
 -  return error (_("Your index file is unmerged."));
 +  return error(_("Your index file is unmerged."));
} else {
unborn = get_sha1("HEAD", head);
if (unborn)
 @@ -756,8 +756,8 @@ static int read_populate_todo(struct commit_list 
**todo_list,
  
fd = open(git_path_todo_file(), O_RDONLY);
if (fd < 0)
 -  return error(_("Could not open %s (%s)"),
 -  git_path_todo_file(), strerror(errno));
 +  return error_errno(_("Could not open %s"),
 + git_path_todo_file());
if (strbuf_read(, fd, 0) < 0) {
close(fd);
strbuf_release();
 @@ -841,8 +841,8 @@ static int create_seq_dir(void)
return -1;
}
else if (mkdir(git_path_seq_dir(), 0777) < 0)
 -  return error(_("Could not create sequencer directory %s (%s)"),
 -git_path_seq_dir(), strerror(errno));
 +  return error_errno(_("Could not create sequencer directory %s"),
 + git_path_seq_dir());
return 0;
  }
  
 @@ -941,8 +941,10 @@ static int save_todo(struct commit_list *todo_list, 
struct replay_opts *opts)
if (fd < 0)
return error_errno(_("Could not lock '%s'"),
   git_path_todo_file());
 -  if (format_todo(, todo_list, opts) < 0)
 +  if (format_todo(, todo_list, opts) < 0) {
 +  strbuf_release();
return error(_("Could not format %s."), git_path_todo_file());
 +  }
if (write_in_full(fd, buf.buf, buf.len) < 0) {
strbuf_release();
return error_errno(_("Could not write to %s"),

-- 
2.10.0.rc1.99.gcd66998

base-commit: 5cb0d5ad05e027cbddcb0a3c7518ddeea0f7c286
--
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 12/14] sequencer: lib'ify save_head()

[Johannes Schindelin]

Instead of dying there, let the caller high up in the callchain notice
the error and handle it (by dying, still).

The only caller of save_head(), sequencer_pick_revisions() can already
return errors, so its caller must be already prepared to handle error
returns, and with this step, we make it notice an error return from
this function.

So this is a safe conversion to make save_head() callable from new
callers that want it not to die, without changing the external
behaviour of anything existing.

Signed-off-by: Johannes Schindelin 
---
 sequencer.c | 15 ++-
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index 9a1f0af..a819919 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -846,18 +846,22 @@ static int create_seq_dir(void)
return 0;
 }
 
-static void save_head(const char *head)
+static int save_head(const char *head)
 {
static struct lock_file head_lock;
struct strbuf buf = STRBUF_INIT;
int fd;
 
-   fd = hold_lock_file_for_update(_lock, git_path_head_file(), 
LOCK_DIE_ON_ERROR);
+   fd = hold_lock_file_for_update(_lock, git_path_head_file(), 0);
+   if (fd < 0)
+   return error_errno(_("Could not lock HEAD"));
strbuf_addf(, "%s\n", head);
if (write_in_full(fd, buf.buf, buf.len) < 0)
-   die_errno(_("Could not write to %s"), git_path_head_file());
+   return error_errno(_("Could not write to %s"),
+  git_path_head_file());
if (commit_lock_file(_lock) < 0)
-   die(_("Error wrapping up %s."), git_path_head_file());
+   return error(_("Error wrapping up %s."), git_path_head_file());
+   return 0;
 }
 
 static int reset_for_rollback(const unsigned char *sha1)
@@ -1121,7 +1125,8 @@ int sequencer_pick_revisions(struct replay_opts *opts)
return -1;
if (get_sha1("HEAD", sha1) && (opts->action == REPLAY_REVERT))
return error(_("Can't revert as initial commit"));
-   save_head(sha1_to_hex(sha1));
+   if (save_head(sha1_to_hex(sha1)))
+   return -1;
save_opts(opts);
return pick_commits(todo_list, opts);
 }
-- 
2.10.0.rc1.99.gcd66998


--
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 10/14] sequencer: lib'ify read_populate_opts()

[Johannes Schindelin]

Instead of dying there, let the caller high up in the callchain notice
the error and handle it (by dying, still).

The only caller of read_populate_opts(), sequencer_continue() can
already return errors, so its caller must be already prepared to
handle error returns, and with this step, we make it notice an error
return from this function.

So this is a safe conversion to make read_populate_opts() callable
from new callers that want it not to die, without changing the
external behaviour of anything existing.

Signed-off-by: Johannes Schindelin 
---
 sequencer.c | 14 --
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index e11b24f..be6020a 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -808,12 +808,14 @@ static int populate_opts_cb(const char *key, const char 
*value, void *data)
return 0;
 }
 
-static void read_populate_opts(struct replay_opts **opts_ptr)
+static int read_populate_opts(struct replay_opts **opts)
 {
if (!file_exists(git_path_opts_file()))
-   return;
-   if (git_config_from_file(populate_opts_cb, git_path_opts_file(), 
*opts_ptr) < 0)
-   die(_("Malformed options sheet: %s"), git_path_opts_file());
+   return 0;
+   if (git_config_from_file(populate_opts_cb, git_path_opts_file(), *opts) 
< 0)
+   return error(_("Malformed options sheet: %s"),
+   git_path_opts_file());
+   return 0;
 }
 
 static int walk_revs_populate_todo(struct commit_list **todo_list,
@@ -1021,8 +1023,8 @@ static int sequencer_continue(struct replay_opts *opts)
 
if (!file_exists(git_path_todo_file()))
return continue_single_pick();
-   read_populate_opts();
-   if (read_populate_todo(_list, opts))
+   if (read_populate_opts() ||
+   read_populate_todo(_list, opts))
return -1;
 
/* Verify that the conflict has been resolved */
-- 
2.10.0.rc1.99.gcd66998


--
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 08/14] sequencer: lib'ify read_and_refresh_cache()

[Johannes Schindelin]

Instead of dying there, let the caller high up in the callchain
notice the error and handle it (by dying, still).

There are two call sites of read_and_refresh_cache(), one of which is
pick_commits(), whose callers were already prepared to do the right
thing given an "error" return from it by an earlier patch, so the
conversion is safe.

The other one, sequencer_pick_revisions() was also prepared to relay
an error return back to its caller in all remaining cases in an
earlier patch.

Signed-off-by: Johannes Schindelin 
---
 sequencer.c | 15 ++-
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index c006cae..e30aa82 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -640,18 +640,21 @@ static int prepare_revs(struct replay_opts *opts)
return 0;
 }
 
-static void read_and_refresh_cache(struct replay_opts *opts)
+static int read_and_refresh_cache(struct replay_opts *opts)
 {
static struct lock_file index_lock;
int index_fd = hold_locked_index(_lock, 0);
if (read_index_preload(_index, NULL) < 0)
-   die(_("git %s: failed to read the index"), action_name(opts));
+   return error(_("git %s: failed to read the index"),
+   action_name(opts));
refresh_index(_index, REFRESH_QUIET|REFRESH_UNMERGED, NULL, NULL, 
NULL);
if (the_index.cache_changed && index_fd >= 0) {
if (write_locked_index(_index, _lock, COMMIT_LOCK))
-   die(_("git %s: failed to refresh the index"), 
action_name(opts));
+   return error(_("git %s: failed to refresh the index"),
+   action_name(opts));
}
rollback_lock_file(_lock);
+   return 0;
 }
 
 static int format_todo(struct strbuf *buf, struct commit_list *todo_list,
@@ -981,7 +984,8 @@ static int pick_commits(struct commit_list *todo_list, 
struct replay_opts *opts)
if (opts->allow_ff)
assert(!(opts->signoff || opts->no_commit ||
opts->record_origin || opts->edit));
-   read_and_refresh_cache(opts);
+   if (read_and_refresh_cache(opts))
+   return -1;
 
for (cur = todo_list; cur; cur = cur->next) {
save_todo(cur, opts);
@@ -1045,7 +1049,8 @@ int sequencer_pick_revisions(struct replay_opts *opts)
if (opts->subcommand == REPLAY_NONE)
assert(opts->revs);
 
-   read_and_refresh_cache(opts);
+   if (read_and_refresh_cache(opts))
+   return -1;
 
/*
 * Decide what to do depending on the arguments; a fresh
-- 
2.10.0.rc1.99.gcd66998


--
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 06/14] sequencer: lib'ify walk_revs_populate_todo()

[Johannes Schindelin]

Instead of dying there, let the caller high up in the callchain notice
the error and handle it (by dying, still).

The function sequencer_pick_revisions() is the only caller of
walk_revs_populate_todo(), and it already returns errors
appropriately, so its caller must be already prepared to handle error
returns, and with this step, we make it notice an error return from
this function.

So this is a safe conversion to make walk_revs_populate_todo()
callable from new callers that want it not to die, without changing
the external behaviour of anything existing.

Signed-off-by: Johannes Schindelin 
---
 sequencer.c | 10 ++
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index 7eef512..ea2681e 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -809,17 +809,19 @@ static void read_populate_opts(struct replay_opts 
**opts_ptr)
die(_("Malformed options sheet: %s"), git_path_opts_file());
 }
 
-static void walk_revs_populate_todo(struct commit_list **todo_list,
+static int walk_revs_populate_todo(struct commit_list **todo_list,
struct replay_opts *opts)
 {
struct commit *commit;
struct commit_list **next;
 
-   prepare_revs(opts);
+   if (prepare_revs(opts))
+   return -1;
 
next = todo_list;
while ((commit = get_revision(opts->revs)))
next = commit_list_append(commit, next);
+   return 0;
 }
 
 static int create_seq_dir(void)
@@ -1102,8 +1104,8 @@ int sequencer_pick_revisions(struct replay_opts *opts)
 * progress
 */
 
-   walk_revs_populate_todo(_list, opts);
-   if (create_seq_dir() < 0)
+   if (walk_revs_populate_todo(_list, opts) ||
+   create_seq_dir() < 0)
return -1;
if (get_sha1("HEAD", sha1) && (opts->action == REPLAY_REVERT))
return error(_("Can't revert as initial commit"));
-- 
2.10.0.rc1.99.gcd66998


--
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 13/14] sequencer: lib'ify save_todo()

[Johannes Schindelin]

Instead of dying there, let the caller high up in the callchain notice
the error and handle it (by dying, still).

The only caller of save_todo(), pick_commits() can already return
errors, so its caller must be already prepared to handle error
returns, and with this step, we make it notice an error return from
this function.

So this is a safe conversion to make save_todo() callable
from new callers that want it not to die, without changing the
external behaviour of anything existing.

Signed-off-by: Johannes Schindelin 
---
 sequencer.c | 22 +++---
 1 file changed, 15 insertions(+), 7 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index a819919..3e877f1 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -931,24 +931,31 @@ static int sequencer_rollback(struct replay_opts *opts)
return -1;
 }
 
-static void save_todo(struct commit_list *todo_list, struct replay_opts *opts)
+static int save_todo(struct commit_list *todo_list, struct replay_opts *opts)
 {
static struct lock_file todo_lock;
struct strbuf buf = STRBUF_INIT;
int fd;
 
-   fd = hold_lock_file_for_update(_lock, git_path_todo_file(), 
LOCK_DIE_ON_ERROR);
-   if (format_todo(, todo_list, opts) < 0)
-   die(_("Could not format %s."), git_path_todo_file());
+   fd = hold_lock_file_for_update(_lock, git_path_todo_file(), 0);
+   if (fd < 0)
+   return error_errno(_("Could not lock '%s'"),
+  git_path_todo_file());
+   if (format_todo(, todo_list, opts) < 0) {
+   strbuf_release();
+   return error(_("Could not format %s."), git_path_todo_file());
+   }
if (write_in_full(fd, buf.buf, buf.len) < 0) {
strbuf_release();
-   die_errno(_("Could not write to %s"), git_path_todo_file());
+   return error_errno(_("Could not write to %s"),
+  git_path_todo_file());
}
if (commit_lock_file(_lock) < 0) {
strbuf_release();
-   die(_("Error wrapping up %s."), git_path_todo_file());
+   return error(_("Error wrapping up %s."), git_path_todo_file());
}
strbuf_release();
+   return 0;
 }
 
 static void save_opts(struct replay_opts *opts)
@@ -997,7 +1004,8 @@ static int pick_commits(struct commit_list *todo_list, 
struct replay_opts *opts)
return -1;
 
for (cur = todo_list; cur; cur = cur->next) {
-   save_todo(cur, opts);
+   if (save_todo(cur, opts))
+   return -1;
res = do_pick_commit(cur->item, opts);
if (res)
return res;
-- 
2.10.0.rc1.99.gcd66998


--
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 14/14] sequencer: lib'ify save_opts()

[Johannes Schindelin]

Instead of dying there, let the caller high up in the callchain notice
the error and handle it (by dying, still).

The only caller of save_opts(), sequencer_pick_revisions() can already
return errors, so its caller must be already prepared to handle error
returns, and with this step, we make it notice an error return from
this function.

So this is a safe conversion to make save_opts() callable from new
callers that want it not to die, without changing the external
behaviour of anything existing.

Signed-off-by: Johannes Schindelin 
---
 sequencer.c | 26 ++
 1 file changed, 14 insertions(+), 12 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index 3e877f1..b6481bb 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -958,37 +958,39 @@ static int save_todo(struct commit_list *todo_list, 
struct replay_opts *opts)
return 0;
 }
 
-static void save_opts(struct replay_opts *opts)
+static int save_opts(struct replay_opts *opts)
 {
const char *opts_file = git_path_opts_file();
+   int res = 0;
 
if (opts->no_commit)
-   git_config_set_in_file(opts_file, "options.no-commit", "true");
+   res |= git_config_set_in_file_gently(opts_file, 
"options.no-commit", "true");
if (opts->edit)
-   git_config_set_in_file(opts_file, "options.edit", "true");
+   res |= git_config_set_in_file_gently(opts_file, "options.edit", 
"true");
if (opts->signoff)
-   git_config_set_in_file(opts_file, "options.signoff", "true");
+   res |= git_config_set_in_file_gently(opts_file, 
"options.signoff", "true");
if (opts->record_origin)
-   git_config_set_in_file(opts_file, "options.record-origin", 
"true");
+   res |= git_config_set_in_file_gently(opts_file, 
"options.record-origin", "true");
if (opts->allow_ff)
-   git_config_set_in_file(opts_file, "options.allow-ff", "true");
+   res |= git_config_set_in_file_gently(opts_file, 
"options.allow-ff", "true");
if (opts->mainline) {
struct strbuf buf = STRBUF_INIT;
strbuf_addf(, "%d", opts->mainline);
-   git_config_set_in_file(opts_file, "options.mainline", buf.buf);
+   res |= git_config_set_in_file_gently(opts_file, 
"options.mainline", buf.buf);
strbuf_release();
}
if (opts->strategy)
-   git_config_set_in_file(opts_file, "options.strategy", 
opts->strategy);
+   res |= git_config_set_in_file_gently(opts_file, 
"options.strategy", opts->strategy);
if (opts->gpg_sign)
-   git_config_set_in_file(opts_file, "options.gpg-sign", 
opts->gpg_sign);
+   res |= git_config_set_in_file_gently(opts_file, 
"options.gpg-sign", opts->gpg_sign);
if (opts->xopts) {
int i;
for (i = 0; i < opts->xopts_nr; i++)
-   git_config_set_multivar_in_file(opts_file,
+   res |= git_config_set_multivar_in_file_gently(opts_file,

"options.strategy-option",
opts->xopts[i], "^$", 
0);
}
+   return res;
 }
 
 static int pick_commits(struct commit_list *todo_list, struct replay_opts 
*opts)
@@ -1133,9 +1135,9 @@ int sequencer_pick_revisions(struct replay_opts *opts)
return -1;
if (get_sha1("HEAD", sha1) && (opts->action == REPLAY_REVERT))
return error(_("Can't revert as initial commit"));
-   if (save_head(sha1_to_hex(sha1)))
+   if (save_head(sha1_to_hex(sha1)) ||
+   save_opts(opts))
return -1;
-   save_opts(opts);
return pick_commits(todo_list, opts);
 }
 
-- 
2.10.0.rc1.99.gcd66998
--
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 04/14] sequencer: lib'ify do_recursive_merge()

[Johannes Schindelin]

Instead of dying there, let the caller high up in the callchain
notice the error and handle it (by dying, still).

The only caller of do_recursive_merge(), do_pick_commit() already
checks the return value and passes it on to its callers, so its caller
must be already prepared to handle error returns, and with this step,
we make it notice an error return from this function.

So this is a safe conversion to make do_recursive_merge() callable
from new callers that want it not to die, without changing the
external behaviour of anything existing.

Signed-off-by: Johannes Schindelin 
---
 sequencer.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/sequencer.c b/sequencer.c
index 17bee60..f6cdf65 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -303,7 +303,8 @@ static int do_recursive_merge(struct commit *base, struct 
commit *next,
if (active_cache_changed &&
write_locked_index(_index, _lock, COMMIT_LOCK))
/* TRANSLATORS: %s will be "revert" or "cherry-pick" */
-   die(_("%s: Unable to write new index file"), action_name(opts));
+   return error(_("%s: Unable to write new index file"),
+   action_name(opts));
rollback_lock_file(_lock);
 
if (opts->signoff)
-- 
2.10.0.rc1.99.gcd66998


--
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 07/14] sequencer: lib'ify prepare_revs()

[Johannes Schindelin]

Instead of dying there, let the caller high up in the callchain notice
the error and handle it (by dying, still).

The only caller of prepare_revs(), walk_revs_populate_todo() was just
taught to return errors, after verifying that its callers are prepared
to handle error returns, and with this step, we make it notice an
error return from this function.

So this is a safe conversion to make prepare_revs() callable from new
callers that want it not to die, without changing the external
behaviour of anything existing.

Signed-off-by: Johannes Schindelin 
---
 sequencer.c | 7 ---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index ea2681e..c006cae 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -623,7 +623,7 @@ static int do_pick_commit(struct commit *commit, struct 
replay_opts *opts)
return res;
 }
 
-static void prepare_revs(struct replay_opts *opts)
+static int prepare_revs(struct replay_opts *opts)
 {
/*
 * picking (but not reverting) ranges (but not individual revisions)
@@ -633,10 +633,11 @@ static void prepare_revs(struct replay_opts *opts)
opts->revs->reverse ^= 1;
 
if (prepare_revision_walk(opts->revs))
-   die(_("revision walk setup failed"));
+   return error(_("revision walk setup failed"));
 
if (!opts->revs->commits)
-   die(_("empty commit set passed"));
+   return error(_("empty commit set passed"));
+   return 0;
 }
 
 static void read_and_refresh_cache(struct replay_opts *opts)
-- 
2.10.0.rc1.99.gcd66998


--
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 11/14] sequencer: lib'ify create_seq_dir()

[Johannes Schindelin]

Instead of dying there, let the caller high up in the callchain notice
the error and handle it (by dying, still).

The only caller of create_seq_dir(), sequencer_pick_revisions() can
already return errors, so its caller must be already prepared to
handle error returns, and with this step, we make it notice an error
return from this function.

So this is a safe conversion to make create_seq_dir() callable from
new callers that want it not to die, without changing the external
behaviour of anything existing.

Signed-off-by: Johannes Schindelin 
---
 sequencer.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index be6020a..9a1f0af 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -841,8 +841,8 @@ static int create_seq_dir(void)
return -1;
}
else if (mkdir(git_path_seq_dir(), 0777) < 0)
-   die_errno(_("Could not create sequencer directory %s"),
- git_path_seq_dir());
+   return error_errno(_("Could not create sequencer directory %s"),
+  git_path_seq_dir());
return 0;
 }
 
-- 
2.10.0.rc1.99.gcd66998


--
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 05/14] sequencer: lib'ify do_pick_commit()

[Johannes Schindelin]

Instead of dying there, let the caller high up in the callchain notice
the error and handle it (by dying, still).

The only two callers of do_pick_commit(), pick_commits() and
single_pick() already check the return value and pass it on to their
callers, so their callers must be already prepared to handle error
returns, and with this step, we make it notice an error return from
this function.

So this is a safe conversion to make do_pick_commit() callable from
new callers that want it not to die, without changing the external
behaviour of anything existing.

While at it, remove the superfluous space.

Signed-off-by: Johannes Schindelin 
---
 sequencer.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/sequencer.c b/sequencer.c
index f6cdf65..7eef512 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -464,7 +464,7 @@ static int do_pick_commit(struct commit *commit, struct 
replay_opts *opts)
 * to work on.
 */
if (write_cache_as_tree(head, 0, NULL))
-   die (_("Your index file is unmerged."));
+   return error(_("Your index file is unmerged."));
} else {
unborn = get_sha1("HEAD", head);
if (unborn)
-- 
2.10.0.rc1.99.gcd66998


--
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 03/14] sequencer: lib'ify write_message()

[Johannes Schindelin]

Instead of dying there, let the caller high up in the callchain
notice the error and handle it (by dying, still).

The only caller of write_message(), do_pick_commit() already checks
the return value and passes it on to its callers, so its caller must
be already prepared to handle error returns, and with this step, we
make it notice an error return from this function.

So this is a safe conversion to make write_message() callable
from new callers that want it not to die, without changing the
external behaviour of anything existing.

Signed-off-by: Johannes Schindelin 
---
 sequencer.c | 21 -
 1 file changed, 12 insertions(+), 9 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index baf6b40..17bee60 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -180,17 +180,20 @@ static void print_advice(int show_hint, struct 
replay_opts *opts)
}
 }
 
-static void write_message(struct strbuf *msgbuf, const char *filename)
+static int write_message(struct strbuf *msgbuf, const char *filename)
 {
static struct lock_file msg_file;
 
-   int msg_fd = hold_lock_file_for_update(_file, filename,
-  LOCK_DIE_ON_ERROR);
+   int msg_fd = hold_lock_file_for_update(_file, filename, 0);
+   if (msg_fd < 0)
+   return error_errno(_("Could not lock '%s'"), filename);
if (write_in_full(msg_fd, msgbuf->buf, msgbuf->len) < 0)
-   die_errno(_("Could not write to %s"), filename);
+   return error_errno(_("Could not write to %s"), filename);
strbuf_release(msgbuf);
if (commit_lock_file(_file) < 0)
-   die(_("Error wrapping up %s."), filename);
+   return error(_("Error wrapping up %s."), filename);
+
+   return 0;
 }
 
 static struct tree *empty_tree(void)
@@ -223,7 +226,7 @@ static int fast_forward_to(const unsigned char *to, const 
unsigned char *from,
 
read_cache();
if (checkout_fast_forward(from, to, 1))
-   exit(128); /* the callee should have complained already */
+   return -1; /* the callee should have complained already */
 
strbuf_addf(, _("%s: fast-forward"), action_name(opts));
 
@@ -564,16 +567,16 @@ static int do_pick_commit(struct commit *commit, struct 
replay_opts *opts)
 head, , opts);
if (res < 0)
return res;
-   write_message(, git_path_merge_msg());
+   res |= write_message(, git_path_merge_msg());
} else {
struct commit_list *common = NULL;
struct commit_list *remotes = NULL;
 
-   write_message(, git_path_merge_msg());
+   res = write_message(, git_path_merge_msg());
 
commit_list_insert(base, );
commit_list_insert(next, );
-   res = try_merge_command(opts->strategy, opts->xopts_nr, 
opts->xopts,
+   res |= try_merge_command(opts->strategy, opts->xopts_nr, 
opts->xopts,
common, sha1_to_hex(head), remotes);
free_commit_list(common);
free_commit_list(remotes);
-- 
2.10.0.rc1.99.gcd66998


--
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 09/14] sequencer: lib'ify read_populate_todo()

[Johannes Schindelin]

Instead of dying there, let the caller high up in the callchain
notice the error and handle it (by dying, still).

The only caller of read_populate_todo(), sequencer_continue() can
already return errors, so its caller must be already prepared to
handle error returns, and with this step, we make it notice an
error return from this function.

So this is a safe conversion to make read_populate_todo() callable
from new callers that want it not to die, without changing the
external behaviour of anything existing.

Signed-off-by: Johannes Schindelin 
---
 sequencer.c | 14 +-
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index e30aa82..e11b24f 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -748,7 +748,7 @@ static int parse_insn_buffer(char *buf, struct commit_list 
**todo_list,
return 0;
 }
 
-static void read_populate_todo(struct commit_list **todo_list,
+static int read_populate_todo(struct commit_list **todo_list,
struct replay_opts *opts)
 {
struct strbuf buf = STRBUF_INIT;
@@ -756,18 +756,21 @@ static void read_populate_todo(struct commit_list 
**todo_list,
 
fd = open(git_path_todo_file(), O_RDONLY);
if (fd < 0)
-   die_errno(_("Could not open %s"), git_path_todo_file());
+   return error_errno(_("Could not open %s"),
+  git_path_todo_file());
if (strbuf_read(, fd, 0) < 0) {
close(fd);
strbuf_release();
-   die(_("Could not read %s."), git_path_todo_file());
+   return error(_("Could not read %s."), git_path_todo_file());
}
close(fd);
 
res = parse_insn_buffer(buf.buf, todo_list, opts);
strbuf_release();
if (res)
-   die(_("Unusable instruction sheet: %s"), git_path_todo_file());
+   return error(_("Unusable instruction sheet: %s"),
+   git_path_todo_file());
+   return 0;
 }
 
 static int populate_opts_cb(const char *key, const char *value, void *data)
@@ -1019,7 +1022,8 @@ static int sequencer_continue(struct replay_opts *opts)
if (!file_exists(git_path_todo_file()))
return continue_single_pick();
read_populate_opts();
-   read_populate_todo(_list, opts);
+   if (read_populate_todo(_list, opts))
+   return -1;
 
/* Verify that the conflict has been resolved */
if (file_exists(git_path_cherry_pick_head()) ||
-- 
2.10.0.rc1.99.gcd66998


--
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 v14 07/27] bisect--helper: `bisect_reset` shell function in C

[Pranit Bauva]

Hey Junio,

On Thu, Aug 25, 2016 at 2:42 AM, Junio C Hamano  wrote:
>
> Pranit Bauva  writes:
>
> > +static int bisect_reset(const char *commit)
> > +{
> > + struct strbuf branch = STRBUF_INIT;
> > +
> > + if (!commit) {
> > + if (strbuf_read_file(, git_path_bisect_start(), 0) < 
> > 1) {
>
> Hmm, tricky but correct to do the "< 1" comparison.  If the file
> does not exist, you'd get -1; if it fails to read it, you'd get -1;
> if it turns out to be empty, you'd get 0.
>
> > + printf("We are not bisecting.\n");
> > + return 0;
> > + }
> > + strbuf_rtrim();
> > + } else {
> > + struct object_id oid;
> > + if (get_oid(commit, ))
> > + return error(_("'%s' is not a valid commit"), commit);
>
> The original is
>
> rev-parse --quiet --verify "$1^{commit}"
>
> Unless the caller of this function already appended "^{commit}" to
> whatever the user gave "bisect--helper bisect-reset", this
> conversion is not equivalent.  If you said
>
> git bisect reset HEAD:
>
> get_oid() would tell you that the top-level tree object of the
> current commit exists in the object store, but the original is
> meant to catch "That's not a commit, it's a tree!" before attempting
> to run "git checkout" on it.
>
> I think get_sha1_committish() is what you want to use here.
>
> > + strbuf_addstr(, commit);
> > + }
>

Ya! get_sha1_committish() would be better. Thanks!

>
> Also this version fails to catch "bisect reset a b c" as an error, I
> suspect.

It didn't when I tried it right now. Could you please elaborate on why
you think it can fail? There might be a thing which I haven't tested.

Regards,
Pranit Bauva
--
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 06/15] sequencer: lib'ify read_populate_todo()

[Johannes Schindelin]

Hi Junio,

On Thu, 25 Aug 2016, Junio C Hamano wrote:

> Johannes Schindelin  writes:
> 
> > To be truly useful, the sequencer should never die() but always return
> > an error.
> >
> > Signed-off-by: Johannes Schindelin 
> > ---
> >  sequencer.c | 14 +-
> >  1 file changed, 9 insertions(+), 5 deletions(-)
> 
> Instead of dying there, you let the caller high up in the callchain
> to notice the error and handle it (by dying).
> 
> The only caller of read_populate_todo(), sequencer_continue() can
> already return errors, so its caller must be already prepared to
> handle error returns, and with this step, you make it notice an
> error return from this function.  So this is a safe conversion to
> make read_populate_todo() callable from new callers that want it not
> to die, without changing the external behaviour of anything
> existing.
> 
> Good.
> 
> By the way, I am writing these as review comments because I do not
> want to keep repeating this kind of analysis as a reviewer.  I am
> demonstrating what should have been in the commit log message
> instead, so that the reviewer does not have to spend extra time, if
> the reviewer trusts the author's diligence well enough, to see if
> the conversion makes sense.
> 
> Please follow the example when/if you have to reroll.  I want the
> patches to show the evidence of careful analysis to reviewers so
> that they can gauge the trustworthiness of the patches.  With this
> round of patches, honestly, I cannot tell if it is a mechanical
> substitution alone, or such a substitution followed by a careful
> verification of the callers.

Duly noted.

I fixed up the patch order as well as the commit messages.

Now on to a request of my own: I am once again reminded that I have *no*
good mail client to serve me. (Before you suggest it: no, emacs won't cut
it for me, nor mutt. Thank you very much for your suggestion.) So it is
really annoying for me to scroll through quoted text, page after page,
only to find that none of it got a response after all.

In short: I would really appreciate it if you could cut quoted text after
your last response.

Thanks,
Dscho

P.S.: It's this mailing list thing again. I would *love* to switch to a
mail client more to my liking, but the ones I like all won't respect white
space in patch files that are pasted verbatim into the mail body.
--
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 04/15] sequencer: lib'ify prepare_revs()

[Johannes Schindelin]

Hi Junio,

On Thu, 25 Aug 2016, Junio C Hamano wrote:

> Johannes Schindelin  writes:
> 
> > To be truly useful, the sequencer should never die() but always return
> > an error.
> >
> > Signed-off-by: Johannes Schindelin 
> > ---
> 
> I am still looking at sequencer.c in 'master', but I do not think
> that the sole caller of this function, walk_revs_populate_todo(),
> is prepared to act on an error return from this function and instead
> it expects this to die() when in trouble.  And I do not think I saw
> the function touched in the steps so far.
> 
> So this step smells like a fishy conversion to me.

The fishy part is, of course, that I managed not to realize that the patch
libifiying walk_revs_populate_todo() came *after* this patch that libified
one of its callees, prepare_revs().

FWIW the same is true for sequencer_pick_revisions(), which was libified
way too late in the game.

This is once again a demonstration that a final patch series does *not*
reflect the process of developing it: in this case, the order of the
patches is the *opposite* of their development, as I libified functions
from the lowest, deepest call depth up to the top-level functions.

Fixed in v2,
Dscho
--
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] checkout: swap the order of ambiguity check for :/ syntax

[Duy Nguyen]

On Fri, Aug 26, 2016 at 12:19 AM, Junio C Hamano  wrote:
> Duy Nguyen  writes:
>
>> On Wed, Aug 24, 2016 at 11:35 PM, Junio C Hamano  wrote:
>>> Nguyễn Thái Ngọc Duy   writes:
>>>
 It's not wonderful, but it's in line with how git-checkout stops caring
 about ambiguity after the first argument can be resolved as a ref
 (there's even a test for it, t2010.6).
>>>
>>> But that is justifiable because checkout can only ever take one
>>> revision.  What follows, if there are any, must be paths, and more
>>> importantly, it would be perfectly reasonable if some of them were
>>> missing in the working tree ("ow, I accidentally removed that file,
>>> I need to resurrect it from the index").  Does the same justification
>>> apply to this change?
>>
>> I think there is a misunderstanding. My "after" is in "after the first
>> argument can be resolved, check if it exists in worktree too, if so
>> it's ambiguous and bail". This is usually how we detect ambiguation.
>> But git-checkout does not do the "check if it exists..." clause.
>
> Hmph.  The "case 4" in the function you touched says
>
>  * case 4: git checkout  
>  *
>  *   The first argument must not be ambiguous.
>  *   - If it's *only* a reference, treat it like case (1).
>  *   - If it's only a path, treat it like case (2).
>  *   - else: fail.
>
> Did we break it recently?

I was driven by testing and did not read the code carefully (but on
the other hand, ambiguation plus dwim is never a pleasant read). Near
the end of this function we have

if (!has_dash_dash) {/* case (3).(d) -> (1) */
/*
 * Do not complain the most common case
 *  git checkout branch
 * even if there happen to be a file called 'branch';
 * it would be extremely annoying.
 */
if (argc)
verify_non_filename(NULL, arg);

argc == 0 would be case 3d, argc != 0 is case 4 and "touch abc; git
checkout :/abc def" does complain about :/abc being ambiguous. So no
we haven't broken anything yet, but I do with my patch. If I want to
make a special case for :/ to speed it up, I need to be careful about
this (and I probably need a test too because it's not detected).

While at there, verify_non_filename() in this function (and
check_filename() too) should pass the right prefix in argument 1, not
NULL, I think. I'll need to do some digging to see why they are NULL
in the first place though.
-- 
Duy
--
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

[Ali Saeed]

Did you get my 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 03/15] sequencer: lib'ify do_pick_commit()

[Johannes Schindelin]

Hi Junio,

On Thu, 25 Aug 2016, Junio C Hamano wrote:

> > if (write_cache_as_tree(head, 0, NULL))
> > -   die (_("Your index file is unmerged."));
> > +   return error (_("Your index file is unmerged."));
> 
> While you are touching the line, it is a good idea to correct an
> obvious style error like this one.  "Do one thing and one thing well
> in a commit" is a good discipline, but it is absurd to take it to
> the extreme.

To be quite honest, I had to look *really* hard to see that space. It took
me literally 30 seconds to spot the style issue.

Fixed in v2,
Dscho
--
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: [RFC] Proposed questions for "Git User's Survey 2016"

[Jakub Narębski]

W dniu 26.08.2016 o 08:15, Eric Wong pisze:

> Not directly-related to the survey questions, but can you ensure
> it's accessible to folks without JavaScript/graphics, and
> perhaps also ensure it is on a host that is Tor-friendly?

I plan on using Survs.com (where we have Premium account for free,
thanks to Survs admins).  It has good online analysis tools, allows
to download results in easy parseable format[1], and has support
for multiple channels.

[1]: https://github.com/jnareb/GitSurvey-scripts


It is possible to have a channel without JavaScript and without
cookies (anonymous), at the cost of having to fill the survey
in one go (without cookies and JavaScript you cannot go back
to fill more questions, or change your mind about answers).
The main page of survey would have a link to JavaScript-free
version, and all announcements (by me) would include it.

There would be no graphics that are not purely optional
(decorative); probably there would be just a logo.

I'm not sure how Tor-friendly is this host.
 
> Graphics setups often require non-Free firmware or drivers for
> acceptable performance; and there are also visually-impaired
> users who will need screen readers or Braille.

A question: would it be better to have the whole survey one one
large page, or have it split into pages (with fever questions
per page)?


P.S. With from 3k to 11k responses (I hope for at least 5k)
Git User's Survey is outside of the free tier of most if not
all web survey sites.  I guess I could cobble something together
out of Google Forms, but we would loose online analysis (and
probably some of data that Survs.com makes available in download).

Best regards,
-- 
Jakub Narębski
--
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: [RFC] Proposed questions for "Git User's Survey 2016"

[Jakub Narębski]

W dniu 26.08.2016 o 08:58, Andrew Ardill pisze:
> Jakub Narębski  wrote:
>> Andrew Ardill pisze:
>>> Jakub Narębski  wrote:
[...]
 25. What [channel(s)] do you use to request/get help about Git [(if any)]
>>>
>>> It may also be useful to ask how people hear news about git, such as
>>> when a new release comes out. Not sure if worth a separate question,
>>> as there is a lot of crossover in the resources available for this and
>>> for requesting help, but knowing this information would help us
>>> understand what kinds of users are responding and which communication
>>> channels are effective for git news.
>>
>> How would you propose such question would look like, and what proposed
>> answers would be (if it were not a free-text / essay question)?
> 
> Something like:
> 
> XX. How do you hear about git related news 
> (such as new releases and community events)?
> (multiple choice or single choice?)
>  * I wasn't aware there was any news
>  * I don't read any news, but I'm aware of it
>  * through news aggregation sites (such as reddit or hacker news)
>  * from a newsletter (such as Git Rev News)

I wonder if we should also add

   * through a watched blog (such as GitHub Blog)
   * announcement section in news site (such as LWN.net)

>  * from a mailing list (such as the git developer or the git for windows list)

Are there any other common channels that you (the people) watch for
git news or software news?  Freshmeat / Freecode is defunct, and no
replacement got popular enough...

>  * other

The last would be

   * other, please specify

> It would be good to allow a list of specific resources to be written
> to capture things we don't know about, and specific instances of the
> categories above, e.g.:
> "hacker news, git mailing list, git rev news"

On Survs.com it would require an additional question with free-form
answer, I think.

>> Note that there might be a problem of severe bias: people who heard
>> about Git User's Survey 2016 are probably ones that watch news about Git.
>> Still, it would be useful to know if people read RelNotes...
> 
> Agreed, the intent behind the question is to work out what are the
> effective communication channels so that they can be used more
> effectively.

Right.

Nb. I wonder if this question should not replace the one about
how you heard about the survey (at least if there would be too many
questions).  But I'll have to see how many questions we have now.

Thanks for suggestions,
-- 
Jakub Narębski

--
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 v5 01/12] doc: use 'symmetric difference' consistently

[Jakub Narębski]

W dniu 11.08.2016 o 23:50, Philip Oakley pisze:
  
> diff --git a/Documentation/rev-list-options.txt 
> b/Documentation/rev-list-options.txt
> index 4f009d4..6dc0bb0 100644
> --- a/Documentation/rev-list-options.txt
> +++ b/Documentation/rev-list-options.txt
> @@ -225,7 +225,7 @@ excluded from the output.
>  
>  --left-only::
>  --right-only::
> - List only commits on the respective side of a symmetric range,
> + List only commits on the respective side of a symmetric difference,
>   i.e. only those which would be marked `<` resp. `>` by
>   `--left-right`.
>  +
> @@ -766,7 +766,7 @@ ifdef::git-rev-list[]
>  endif::git-rev-list[]
>  
>  --left-right::
> - Mark which side of a symmetric diff a commit is reachable from.
> + Mark which side of a symmetric difference a commit is reachable from.
>   Commits from the left side are prefixed with `<` and those from
>   the right with `>`.  If combined with `--boundary`, those
>   commits are prefixed with `-`.

That's very nice that two^W three related options, namely --left-only,
--right-only and --left-right now use the same notation.

I guess that "symmetric range" was to mean "symmetric difference range".

Best,
-- 
Jakub Narębski

--
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 v6 06/13] pkt-line: add functions to read/write flush terminated packet streams

[Lars Schneider]

> On 26 Aug 2016, at 00:27, Junio C Hamano  wrote:
> 
> larsxschnei...@gmail.com writes:
> 
>> From: Lars Schneider 
>> 
>> packet_write_stream_with_flush_from_fd() and
>> packet_write_stream_with_flush_from_buf() write a stream of packets. All
>> content packets use the maximal packet size except for the last one.
>> After the last content packet a `flush` control packet is written.
>> packet_read_till_flush() reads arbitrary sized packets until it detects
>> a `flush` packet.
> 
> These are awkwardly named and I couldn't guess what the input is (I
> can tell one is to read from fd and the other is  buffer,
> but it is unclear if that is in packetized form or just raw data
> stream to be copied to the end from their names) without reading the
> implementation.  I _think_ you read a raw stream of data through the
> end (either EOF or length limit) and write it out packetized, and
> use the flush packet to mark the end of the stream.  In my mind,
> that is "writing a packetized stream".  The words "packetizing" and
> "stream" imply that the stream could consist of more data than what
> would fit in a single packet, which in turn implies that there needs
> a way to mark the end of one data item, so with_flush does not
> necessarily have to be their names.
> 
> The counter-part would be "reading a packetized stream".
> 
>> +int packet_write_stream_with_flush_from_fd(int fd_in, int fd_out)
>> +{
> 
> Especially this one I am tempted to suggest "copy-to-packetized-stream",
> as it reads a stream from one fd and then copies out while packetizing.

OK, what function names would be more clear from your point of view?

copy_to_packetized_stream_from_fd()
copy_to_packetized_stream_from_buf()
copy_to_packetized_stream_to_buf()

or

write_packetized_stream_from_fd()
write_packetized_stream_from_buf()
read_packetized_stream_to_buf()

?


>> +int packet_write_stream_with_flush_from_buf(const char *src_in, size_t len, 
>> int fd_out)
>> +{
>> +int err = 0;
>> +size_t bytes_written = 0;
>> +size_t bytes_to_write;
>> +
>> +while (!err) {
>> +if ((len - bytes_written) > sizeof(packet_write_buffer) - 4)
>> +bytes_to_write = sizeof(packet_write_buffer) - 4;
>> +else
>> +bytes_to_write = len - bytes_written;
>> +if (bytes_to_write == 0)
>> +break;
> 
> The lack of COPY_WRITE_ERROR puzzled me briefly here.  If you are
> assuming that your math at the beginning of this loop is correct and
> bytes_to_write will never exceed the write-buffer size, I think you
> should be able to (and it would be better to) assume that the math
> you do to tell xread() up to how many bytes it is allowed to read at
> once is also correct, losing the COPY_WRITE_ERROR check in the other
> function.  You can choose to play safer and do a check in this
> function, too.  Either way, we would want to be consistent.

OK. I'll remove the (I just realized meaningless) check in the other function:

+   if (bytes_to_write > sizeof(packet_write_buffer) - 4)
+   return COPY_WRITE_ERROR;

> 
>> +err = packet_write_gently(fd_out, src_in + bytes_written, 
>> bytes_to_write);
>> +bytes_written += bytes_to_write;
>> +}
>> +if (!err)
>> +err = packet_flush_gently(fd_out);
>> +return err;
>> +}
> 
>> +ssize_t packet_read_till_flush(int fd_in, struct strbuf *sb_out)
>> +{
>> +int len, ret;
>> +int options = PACKET_READ_GENTLE_ON_EOF;
>> +char linelen[4];
>> +
>> +size_t oldlen = sb_out->len;
>> +size_t oldalloc = sb_out->alloc;
>> +
>> +for (;;) {
>> +/* Read packet header */
>> +ret = get_packet_data(fd_in, NULL, NULL, linelen, 4, options);
>> +if (ret < 0)
>> +goto done;
>> +len = packet_length(linelen);
>> +if (len < 0)
>> +die("protocol error: bad line length character: %.4s", 
>> linelen);
>> +if (!len) {
>> +/* Found a flush packet - Done! */
>> +packet_trace("", 4, 0);
>> +break;
>> +}
>> +len -= 4;
>> +
>> +/* Read packet content */
>> +strbuf_grow(sb_out, len);
>> +ret = get_packet_data(fd_in, NULL, NULL, sb_out->buf + 
>> sb_out->len, len, options);
>> +if (ret < 0)
>> +goto done;
>> +if (ret != len) {
>> +error("protocol error: incomplete read (expected %d, 
>> got %d)", len, ret);
>> +goto done;
>> +}
>> +
>> +packet_trace(sb_out->buf + sb_out->len, len, 0);
> 
> All of the above seems to pretty much duplicate the logic in
> packet_read(), except that this user does not need options handling
> it has.  Is optimizing that out the reason why you open-coded 

Re: [PATCH v6 05/13] pkt-line: add packet_write_gently()

[Lars Schneider]

> On 25 Aug 2016, at 23:50, Junio C Hamano  wrote:
> 
> larsxschnei...@gmail.com writes:
> 
>> From: Lars Schneider 
>> 
>> packet_write_fmt() has two shortcomings. First, it uses format_packet()
>> which lets the caller only send string data via "%s". That means it
>> cannot be used for arbitrary data that may contain NULs. Second, it will
>> always die on error.
> 
> As you introduced _gently in 3/13, the latter is no longer a valid
> excuse to add this function.  Just remove the sentence "Second, ...".

Agreed!


>> Add packet_write_gently() which writes arbitrary data and returns `0`
>> for success and `-1` for an error. This function is used by other
>> pkt-line functions in a subsequent patch.
>> 
>> Signed-off-by: Lars Schneider 
>> ---
>> pkt-line.c | 12 
>> 1 file changed, 12 insertions(+)
>> 
>> diff --git a/pkt-line.c b/pkt-line.c
>> index cad26df..7e8a803 100644
>> --- a/pkt-line.c
>> +++ b/pkt-line.c
>> @@ -3,6 +3,7 @@
>> #include "run-command.h"
>> 
>> char packet_buffer[LARGE_PACKET_MAX];
>> +static char packet_write_buffer[LARGE_PACKET_MAX];
>> static const char *packet_trace_prefix = "git";
>> static struct trace_key trace_packet = TRACE_KEY_INIT(PACKET);
>> static struct trace_key trace_pack = TRACE_KEY_INIT(PACKFILE);
>> @@ -155,6 +156,17 @@ int packet_write_fmt_gently(int fd, const char *fmt, 
>> ...)
>>  return (write_in_full(fd, buf.buf, buf.len) == buf.len ? 0 : -1);
>> }
>> 
>> +int packet_write_gently(const int fd_out, const char *buf, size_t size)
>> +{
>> +if (size > sizeof(packet_write_buffer) - 4)
>> +return -1;
>> +packet_trace(buf, size, 1);
>> +memmove(packet_write_buffer + 4, buf, size);
>> +size += 4;
>> +set_packet_header(packet_write_buffer, size);
> 
> It may not matter all that much, but from code-reader's point of
> view, when you know packet_write_buffer[] will contain things A and
> B in this order, and when you have enough information to compute A
> before stasrting to fill packet_write_buffer[], I would prefer to
> see you actually fill the buffer in that natural order.

I did that because when packet_write_stream_with_flush_from_fd()
calls packet_write_gently() then buf[] is actually packet_write_buffer[]:

https://github.com/larsxschneider/git/blob/d474e6a4c2523b87624a07111eb7a4f2dcd12426/pkt-line.c#L185-L192

Therefore I would override the first 4 bytes. However, the code evolved for
some reason in that way but looking at it now I think that is an obscure,
likely meaningless optimization. I'll use a separate buffer in 
packet_write_stream_with_flush_from_fd() and then fix the order here
following your advice.


> Do you anticipate future need of non-gently variant of this
> function?  If so, perhaps a helper that takes a boolean "am I
> working for the gently variant?" may help share more code.

With helper you mean "an additional boolean parameter"? I don't 
see a need for a non-gently variant right now but I will
add this parameter if you think it is a good idea. How would the
signature look like?

int packet_write_gently(const int fd_out, const char *buf, size_t size, int 
gentle)

This would follow type_from_string_gently() in object.h.

Thanks,
Lars--
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 v6 03/13] pkt-line: add packet_write_fmt_gently()

[Lars Schneider]

> On 25 Aug 2016, at 23:41, Junio C Hamano  wrote:
> 
> larsxschnei...@gmail.com writes:
> 
>> From: Lars Schneider 
>> 
>> packet_write_fmt() would die in case of a write error even though for
>> some callers an error would be acceptable. Add packet_write_fmt_gently()
>> which writes a formatted pkt-line and returns `0` for success and `-1`
>> for an error.
>> 
>> Signed-off-by: Lars Schneider 
>> ---
>> pkt-line.c | 12 
>> pkt-line.h |  1 +
>> 2 files changed, 13 insertions(+)
>> 
>> diff --git a/pkt-line.c b/pkt-line.c
>> index e8adc0f..3e8b2fb 100644
>> --- a/pkt-line.c
>> +++ b/pkt-line.c
>> @@ -137,6 +137,18 @@ void packet_write_fmt(int fd, const char *fmt, ...)
>>  write_or_die(fd, buf.buf, buf.len);
>> }
>> 
>> +int packet_write_fmt_gently(int fd, const char *fmt, ...)
>> +{
>> +static struct strbuf buf = STRBUF_INIT;
>> +va_list args;
>> +
>> +strbuf_reset();
>> +va_start(args, fmt);
>> +format_packet(, fmt, args);
>> +va_end(args);
>> +return (write_in_full(fd, buf.buf, buf.len) == buf.len ? 0 : -1);
>> +}
> 
> Even though its only a handful lines, it is a bit ugly to have a
> completely copied implementation only to have _gently().  I suspect
> that you should be able to
> 
>   static int packet_write_fmt_1(int fd, int gently,
>   const char *fmt, va_list args)
>{
>   struct strbuf buf = STRBUF_INIT;
>   size_t count;
> 
>   format_packet(, fmt, args);
>   
>   count = write_in_full(fd, buf.buf, buf.len);
>if (count == buf.len)
>   return 0;
>   if (!gently) {
>   check_pipe(errno);
>   die_errno("write error");
>   }
>return -1;
>   }
> 
> and then share that between the existing one:
> 
>   void packet_write_fmt(int fd, const char *fmt, ...)
>{
>   va_list args;
>   va_start(args, fmt);
>packet_write_fmt_1(fd, 0, fmt, args);
>va_end(args);
>   }
> 
> and the new one:
> 
>   void packet_write_fmt_gently(int fd, const char *fmt, ...)
>{
>   int status;
>   va_list args;
>   va_start(args, fmt);
>status = packet_write_fmt_1(fd, 1, fmt, args);
>va_end(args);
>   return status;
>   }

I agree with your criticism of the code duplication. 

However, I thought it would be OK, as Peff already 
tried to refactor it...
http://public-inbox.org/git/20160810150139.lpxyrqkr53s5f...@sigill.intra.peff.net/

... and I got the impression you agreed with Peff:
http://public-inbox.org/git/xmqqvaz84g9y@gitster.mtv.corp.google.com/


I will try to refactor it according to your suggestion above. 
Would "packet_write_fmt_1()" be an acceptable name or should 
I come up with something more expressive?

Thanks you,
Lars--
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: Working with public-inbox.org [Was: [PATCH] rev-parse: respect core.hooksPath in --git-path]

[Johannes Schindelin]

Hi Arif,

On Thu, 25 Aug 2016, Arif Khokar wrote:

> On 08/25/2016 09:01 AM, Johannes Schindelin wrote:
> >
> > On Thu, 25 Aug 2016, Arif Khokar wrote:
> 
> >>> I considered recommending this as some way to improve the review
> >>> process.  The problem, of course, is that it is very easy to craft
> >>> an email with an innocuous patch and then push some malicious patch
> >>> to the linked repository.
> >>
> >> It should be possible to verify the SHA1 of the blob before and after
> >> the patch is applied given the values listed near the beginning of
> >> the git diff output.
> >
> > There is no guarantee that the SHA-1 has not been tampered with.
> 
> I was implying that the resulting SHA1 of the blob after the malicious
> patch was applied would differ compared to the resulting blob after
> applying the innocuous patch.  Even if you alter the SHA1 value within
> the patch itself, it doesn't change the SHA1 of the result (unless
> you're able to get a hash collision).
> 
> But, if you want to guarantee that the SHA1 hasn't been tampered in the
> email, you could sign it with your private GPG key and others could
> verify the signature with your public key (assuming the web-of-trust
> applies).

Given that I try to convince my fellow core Git developers to adopt an
*easier* patch submission process, that wastes less of contributors' time,
I would be strongly opposed to requiring a web of trust and GPG signatures
just to be able to submit patches to git.git.

Ciao,
Johannes
--
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 v1 0/3] Update eol documentation

[Torsten Bögershausen]

On 25/08/16 22:31, Junio C Hamano wrote:

[]



This [0/3] is meant to be a cover for [1/2] and [2/2]?

I am trying to see if we broke format-patch recently, or it is a
manual editing error.  The latter I do not care about; the former I
do.



No worry, 0/3 is patched by me by hand, sorry for the confusion.

--
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: [RFC] Proposed questions for "Git User's Survey 2016"

[Andrew Ardill]

Hi,

Jakub Narębski  wrote:
> Andrew Ardill pisze:
> > Jakub Narębski  wrote:
> >> 25. What [channel(s)] do you use to request/get help about Git [(if any)]
> >
> > It may also be useful to ask how people hear news about git, such as
> > when a new release comes out. Not sure if worth a separate question,
> > as there is a lot of crossover in the resources available for this and
> > for requesting help, but knowing this information would help us
> > understand what kinds of users are responding and which communication
> > channels are effective for git news.
>
> How would you propose such question would look like, and what proposed
> answers would be (if it were not a free-text / essay question)?

Something like:

XX. How do you hear about git related news (such as new releases and
community events)?
(multiple choice or single choice?)
 * I wasn't aware there was any news
 * I don't read any news, but I'm aware of it
 * through news aggregation sites (such as reddit or hacker news)
 * from a newsletter (such as Git Rev News)
 * from a mailing list (such as the git developer or the git for windows list)
 * other

It would be good to allow a list of specific resources to be written
to capture things we don't know about, and specific instances of the
categories above, eg:
"hacker news, git mailing list, git rev news"

> Note that there might be a problem of severe bias: people who heard
> about Git User's Survey 2016 are probably ones that watch news about Git.
> Still, it would be useful to know if people read RelNotes...

Agreed, the intent behind the question is to work out what are the
effective communication channels so that they can be used more
effectively.


> That is, if people have a pattern to their upgrade of Git, and can
> tell how often they upgrade.
>
> XX. How often you upgrade Git?
> (multiple choice or single choice?)
>
>  * as soon as new version is released
>  * when there is new binary package / distribution package
>  * when updating distribution / system
>  * around every month, or more often
>  * around every 6 months or more often
>  * I use what is installed on system
>
> Something like that?

Exactly, looks great.

Regards,

Andrew Ardill
--
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] for-each-ref: add %(upstream:gone) to mark missing refs

[Karthik Nayak]

Hello,

>> On Thu, Aug 25, 2016 at 12:03 AM, Jeff King  wrote:
>>>
>>> Ah, right. I was feeling like this was all vaguely familiar. I think
>>> it would be better to push forward kn/ref-filter-branch-list.
>>> According to the last "what's cooking", I think that topic is waiting
>>> on more review. If you're willing and able to do so, that would be a
>>> big help.
>>>
>>
>> It's been waiting for review for a _long_ time now.
>>
>
> To be perfectly honest my C skills and familiarity with the git source
> code is not much to speak of. I very much want to take a close look but
> I cannot promise anything worth your time...
>
> But if I do find something I'd like to point out should I just reply
> directly to the e-mails containing the patches as one usually does even
> though they're months old at this point?
>

Not that your review would be absolute but it definitely would be a start.

Replying directly to the patches is the way to go I feel.

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