Re: Working with public-inbox.org [Was: [PATCH] rev-parse: respect core.hooksPath in --git-path]

2016-08-27 Thread Jakub Narębski
W dniu 25.08.2016 o 14:58, Johannes Schindelin pisze:
> On Mon, 22 Aug 2016, Eric Wong wrote:
>> Johannes Schindelin  wrote:
>>
>>> I just want developers who are already familiar with Git, and come up with
>>> an improvement to Git itself, to be able to contribute it without having
>>> to pull out their hair in despair.
>>
>> We want the same thing.  I just want to go farther and get
>> people familiar with (federated|decentralized) tools instead of
>> proprietary and centralized ones.
> 
> Why require users to get familiar with (federated|decentralized) tools
> *unless* they make things provably more convenient? So far, I only see
> that this would add to the hurdle, not improve things.

Arguably for some federated/decentralized tools are preferred
(for philosophical reasons), even if they do not achieve even feature
parity with centralized tools (c.f. FSF).  Though Git is not there
to improve the world, just to be better...

On the other hand some may say that centralized tools (such as GitHub
and its pull requests) do not achieve feature parity with email based
workflow... though the basics are here.

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

2016-08-27 Thread Jakub Narębski
W dniu 22.08.2016 o 15:15, Duy Nguyen pisze:
> On Mon, Aug 22, 2016 at 8:06 PM, Johannes Schindelin
>  wrote:
>>
>> My point stands. We are way more uninviting to contributors than
>> necessary. And a huge part of the problem is that we require contributors
>> to send their patches inlined into whitespace-preserving mails.
> 
> We probably can settle this in the next git survey with a new
> question: what's stopping you from contributing to git?

Added to second take on proposed questions for Git User's Survey 2016
https://public-inbox.org/git/ae804c55-d145-fc90-e1a9-6ebd6c604...@gmail.com/T/#u
'[RFCv2] Proposed questions for "Git User's Survey 2016", take two'

Message-ID: <91a2ffbe-a73b-fbb9-96da-9aea4d439...@gmail.com>
 
-- 
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


[RFCv2] Proposed questions for "Git User's Survey 2016", take two

2016-08-27 Thread Jakub Narębski
Hello,

As I wrote previously here, I am thinking about returning to doing the
Git User's Survey this year.

  Message-ID: <577e6f32.7020...@gmail.com>

  https://marc.info/?l=git=146790381602547=2
  https://public-inbox.org/git/577E6F32.7020007%40gmail.com/

In this email I'd like to propose the revised list of questions (and
answers) for Git User's Survey 2016, following comments to the previous
revision

  Message-ID: <91a2ffbe-a73b-fbb9-96da-9aea4d439...@gmail.com>

  https://public-inbox.org/git/91a2ffbe-a73b-fbb9-96da-9aea4d439...@gmail.com/

Below is my second proposal; old comments are prefixed with "jn> ", new ones
are prefixed with "JN> ".


 About you

jn> This section gives us a bit of demographic information about survey
jn> responders.  Is it useful?  Should we keep it?


01. What country do you live in? (Country of residence, in English)
(free-form single line)

jn> Survs.com do not offer list of countries as a pre-defined drop-down
jn> list (select, with search), and it looks like it is not as easy as
jn> I thought (though I could push responsibility to Locale::Country ;-):
jn>
jn>   https://en.wikipedia.org/wiki/List_of_sovereign_states
jn>
jn> This question originally read "What country are you from?"
jn> which might be thought as country of birth... which country
jn> may not exist any longer, like East Germany.


02. How old are you (in years)?
(single number, or single choice)

jn> Perhaps a selection of age ranges would be better (if less precise).
jn> We could follow the example of StackOverflow Survey here, see below.
JN> Though oldest / youngest info could be interesting...

  * <20
  *  20-24
  *  25-29
  *  30-34
  *  35-39
  *  40-49
  *  50-59
  * >60


03. Your gender
(single choice)

  *  Man
  *  Woman
  *  Other
  *  Prefer to not disclose

jn> This would be new question, to check if we good good diversity
jn> among survey responders (and not skewed in particular direction).
jn> Though I am not sure if it is a good idea...


04. How would you describe your occupation / role as Git user?
(multiple choice with other, limit to 3 selections (?))

  + Developer
  + Programmer
  + Engineer
  + Analyst
  + Manager / Leader
  + Maintainer / Reviewer / Sub-maintainer
  + DevOps
  + Administrator
  + Designer
  + Artist / Writer
  + Tester / QA
  + Expert
  + Student
  + Researcher
  + Teacher
  + Other, please specify

jn> This is a new question, based somewhat on "Developer Occupations"
jn> and "Programmers, Engineers, and Developers" questions in 
jn> StackOverflow Survey.  It is here to find out if different
jn> occupation leads to different ways of using Git, and if there
jn> is some occupation that is not well served by Git.
jn>
jn> The list of "occupations" is preliminary, and I would like to
jn> ask for your thought about possible answers.
  

05. Have you ever contributed to Git project (code, documentation, i18n, etc.)?
(single choice)

  * Yes
  * No
  * Maybe
  # Tried to <--- new answer

jn> This is here to correlate other responses with Git developers.
jn> In 2012 it was called "Does Git include code or documentation by you?
jn> (Are you a Git developer?)".  I think this way of stating it is better;
jn> if it is here to stay - the number of active and past developers is
jn> not very large.  On the other hand it can be used to detect if we
jn> have a bias due to the way survey is announced.

JN> This would also serve as a reminder that Git is an open-source
JN> project (and free software), and anyone can contribute to it.
JN> Should there be a similar question about doing review?


06. What's stopping you from contributing to Git?
What was hardest / most difficult when contribution to Git?
(free-text essay question)

JN> This was the question proposed by Duy Nguyen in "Working with
JN> public-inbox.org" thread (well, the first part of it):
JN>   
https://public-inbox.org/git/CACsJy8BG63oaLbw0f7try3OpzdphLC7UGAaJ=vgikeb36pa...@mail.gmail.com/


jn> NOTE: earlier surveys have had two additional questions that were
jn> since removed (to make survey shorter, among others).  Those were:
jn>
jn>  - What is your preferred [non-programming] language?
jn>  - Which programming languages / technologies you are proficient with?
jn>
jn> Well, the latter was only about programming languages, originally.
JN> And it would be possibly added, of in later section ("Other tools").


 Getting started with Git

jn> This is probably not the best name for this section of questions.
JN> Can anyone think of a better one?


07. Have you found Git easy to learn?
(single choice)

  * Very easy
  * Easy
  * Reasonably easy
  * Hard
  * Very hard

08. Have you found Git easy to use?
(single choice)

  * Very easy
  * Easy
  * Reasonably easy
  * Hard
  * Very hard

jn> Those two questions, considered alone, doesn't tell us much.  If one
jn> uses git, then usually one does 

Re: [PATCH v14 09/27] bisect--helper: `bisect_write` shell function in C

2016-08-27 Thread Junio C Hamano
Pranit Bauva  writes:

>>> +struct bisect_terms {
>>> + struct strbuf term_good;
>>> + struct strbuf term_bad;
>>> +};
>>
>> I think "struct strbuf" is overrated.  ...
>> I think you can just say "const char *" in this case.
>
> Using struct strbuf is not really overrated but in fact required. But

Nothing is required.

I can make your life easier by requiring you to never use struct
strbuf as a structure field type, though.  You will almost never
need it unless you are building something exotic anyway.

Step back and think what "strbuf" is good for.  It holds a pointer
to a piece of memory the field owns, over-allocates and knows the
size of allocation and usage.  That is good if you need to

 (1) frequently find out the length of the string; without a
 separate .len member you would have to run strlen().

 (2) incrementally add to the string in-place; as it overallocates,
 appending to the string would not have to involve realloc()
 every time and the cost of it is amortized.

 (3) make complex operations like splicing another string in,
 trimming substring out, etc.

You need to do none of the above to these fields.  term.term_good is
either taken from an argv[] element, or you read from a line from a
file and set it.  You may do some trivial computation and set it to
the result, like "the other field uses 'old', so this one need to be
set to 'new'".  The user of the field either has the string and sets
it there, or reads the field's value as a whole string.  No string
manipulation in the field in-place is needed.

> yes, for this patch it might seem as overrated. In the shell code
> initally TERM_GOOD is set to "good" while TERM_BAD is set to "bad".
> Now there are a lot of instances (one of which is bisect_start()
> function) where this can change. So if we keep it as "const char *",
> it would be right to change the value of it after wards. And we cannot
> keep it as "char []" because we don't know its size before hand.

You are not making sense.  Nobody would suggest to use a fixed-size
char array in this structure.  That wouldn't even work in the case
where you are stuffing what comes in an argv[] element in there,
e.g.

terms.term_good = argv[3];

And you can of course support changing the value of the field
without using strbuf.  Just update the pointer to point at the piece
of memory that holds the new value.

In short, I do not see any good reason why the term_good field has
to be anything other than "char *term_good" or "const char *term_good".

Now, what you need to consider choosing between the two depends on
where these strings can come from.  If they are known to be always
unchanging between the time you set it til the end of the program
(e.g. using an element in argv[]), you can just assign without
making any copy and you can use "const char *term_good".  All other
cases, the structure needs to take the ownership, and you would need
to make a copy if you don't have the ownership, e.g.

terms.term_good = xstrdup(argv[3]);

You may be reading from a file, a line at a time and you may have a
line's content in a strbuf.  You do not (yet) own the buffer after
reading it, e.g.

strbuf_getline(, fp);
terms.term_good = strbuf_detach(, NULL);

Of course, if you need to take ownership of the memory, you would
need to free(3) it as needed, which means the pattern to set the
field would become

free(terms.term_good);
terms.term_good = ... some new value ...;

Using strbuf as a local variable is good.  It gives a higher level
of abstraction when you are actually performing string operations.
In most applications, however, a field in a struct is where the
result of a step of computation is kept, not a scratch-pad to
perform steps of computation in.  When you are ready to update the
value of a field, you _have_ a completed string, and you can just
use "char *" field to point at it.  There is no need for strbuf in
the field.

Don't look at the data structure used in trailer.[ch] as a model; it
is an example of a terribly bad implementation taste, a pattern that
should not be followed.  Print it, not read it and burn it as a good
symbolic gesture.


--
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 13/27] bisect--helper: `bisect_start` shell function partially in C

2016-08-27 Thread Junio C Hamano
Pranit Bauva  writes:

>> I wonder the whole thing above is better restructured to avoid
>> repeated checks of the same thing.
>>
>> if (is it 40-hex, i.e. detached?) {
>> stuff it to start_head;
>> } else if (skip_prefix(head, "refs/heads/", )) {
>> do the "cogito" check;
>> stuff it to start_head;
>> } else {
>> that's a strange symbolic ref HEAD you have there;
>> }
>
> I guess it changes the behaviour. Its a strange symbolic ref if it
> does not start with "refs/heads".

I did not think my suggestion would change the behaviour at all.  It
would change the code structure a bit to make it more readable, 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


Re: [PATCH v14 21/27] bisect--helper: `bisect_log` shell function in C

2016-08-27 Thread Pranit Bauva
Hey Junio,

On Sat, Aug 27, 2016 at 4:37 AM, Junio C Hamano  wrote:
> 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()?

That is actually much better. Thanks!

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: Crash when using git blame on untracked file

2016-08-27 Thread Thomas Gummerer
Hi,

On 08/27, Simon Ruderich wrote:
> Hello,
> 
> I'm seeing the following crash with Git 2.9.3 on Debian sid
> (amd64):
> 
> $ git init foo
> $ cd foo
> $ touch x
> $ git add x
> $ git commit -m test
> $ touch x.conf
> $ git blame x.conf
> segmentation fault
> 
> I've tested it on Debian stable's 2.1.4 which works fine:
> 
> $ git blame x.conf
> fatal: no such path 'x.conf' in HEAD
> 
> It requires the blamed file to be present, but in some cases it
> works only if the file e.g. "x" is already tracked in Git and the
> other file is called "x.conf" (".conf"-suffix). But in an empty
> repo it seems to happen always.
> 
> Sadly Debian's git has no dbg-package, so the stacktrace is not
> very useful:
> 
> #0  __strcmp_sse2_unaligned () at 
> ../sysdeps/x86_64/multiarch/strcmp-sse2-unaligned.S:31
> #1  0x0041ad7a in ?? ()
> #2  0x00406171 in ?? ()
> #3  0x00405321 in ?? ()
> #4  0x76f9f700 in __libc_start_main (main=0x4051c0, argc=0x3, 
> argv=0x7fffe1a8, init=, fini=, 
> rtld_fini=, stack_end=0x7fffe198)
> at ../csu/libc-start.c:291
> #5  0x004057d9 in ?? ()
> 

Thank you very much for the bug report.  A proposed fix for it below:

--- >8 ---
Subject: [PATCH] blame: fix segfault on untracked files

Since 3b75ee9 ("blame: allow to blame paths freshly added to the index",
2016-07-16) git blame also looks at the index to determine if there is a
file that was freshly added to the index.

cache_name_pos returns -pos - 1 in case there is no match is found, or
if the name matches, but the entry has a stage other than 0.  As git
blame should work for unmerged files, it uses strcmp to determine
whether the name of the returned position matches, in which case the
file exists, but is merely unmerged, or if the file actually doesn't
exist in the index.

If the repository is empty, or if the file would lexicographically be
sorted as the last file in the repository, -cache_name_pos - 1 is
outside of the length of the active_cache array, causing git blame to
segfault.  Guard against that, and die() normally to restore the old
behaviour.

Reported-by: Simon Ruderich 
Signed-off-by: Thomas Gummerer 
---
 builtin/blame.c  | 3 ++-
 t/t8002-blame.sh | 7 +++
 2 files changed, 9 insertions(+), 1 deletion(-)

diff --git a/builtin/blame.c b/builtin/blame.c
index 7ec7823..a5bbf91 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -2244,7 +2244,8 @@ static void verify_working_tree_path(struct commit 
*work_tree, const char *path)
pos = cache_name_pos(path, strlen(path));
if (pos >= 0)
; /* path is in the index */
-   else if (!strcmp(active_cache[-1 - pos]->name, path))
+   else if (-1 - pos < active_nr &&
+!strcmp(active_cache[-1 - pos]->name, path))
; /* path is in the index, unmerged */
else
die("no such path '%s' in HEAD", path);
diff --git a/t/t8002-blame.sh b/t/t8002-blame.sh
index ff09ace..7983bb7 100755
--- a/t/t8002-blame.sh
+++ b/t/t8002-blame.sh
@@ -6,6 +6,13 @@ test_description='git blame'
 PROG='git blame -c'
 . "$TEST_DIRECTORY"/annotate-tests.sh
 
+test_expect_success 'blame untracked file in empty repo' '
+   touch untracked &&
+   test_must_fail git blame untracked 2>actual.err &&
+   echo "fatal: no such path '\''untracked'\'' in HEAD" >expected.err &&
+   test_cmp expected.err actual.err
+'
+
 PROG='git blame -c -e'
 test_expect_success 'blame --show-email' '
check_count \
-- 
2.8.4.1.g3b75ee9

--
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 13/27] bisect--helper: `bisect_start` shell function partially in C

2016-08-27 Thread Pranit Bauva
Hey Junio,

On Fri, Aug 26, 2016 at 12:32 AM, Junio C Hamano  wrote:
> Pranit Bauva  writes:
>
>> +static int bisect_start(struct bisect_terms *terms, int no_checkout,
>> + const char **argv, int argc)
>> +{
>> + int i, has_double_dash = 0, must_write_terms = 0, bad_seen = 0;
>> + int flags, pathspec_pos;
>> + struct string_list revs = STRING_LIST_INIT_DUP;
>> + struct string_list states = STRING_LIST_INIT_DUP;
>
> The original has a single state, not states.  Let's see if there is
> a reason behind the name change
>
>> + unsigned char sha1[20];
>> + struct object_id oid;
>
> More on these below...
>
>> + ...
>> + for (i = 0; i < argc; i++) {
>> + const char *commit_id = xstrfmt("%s^{commit}", argv[i]);
>> + if (!strcmp(argv[i], "--")) {
>> + has_double_dash = 1;
>> + break;
>> + } else if (!strcmp(argv[i], "--no-checkout"))
>> + no_checkout = 1;
>> + else if (!strcmp(argv[i], "--term-good") ||
>> + ...
>> + } else if (starts_with(argv[i], "--") &&
>> +  !one_of(argv[i], "--term-good", "--term-bad", NULL)) {
>> + string_list_clear(, 0);
>> + string_list_clear(, 0);
>> + die(_("unrecognised option: '%s'"), argv[i]);
>> + } else if (get_oid(commit_id, ) && has_double_dash) {
>> + string_list_clear(, 0);
>> + string_list_clear(, 0);
>> + die(_("'%s' does not appear to be a valid revision"), 
>> argv[i]);
>> + } else {
>> + string_list_append(, oid_to_hex());
>> + }
>> + }
>
> What I do not understand is clearing the string list "states" inside
> this loop.  It may have been INIT_DUPed, but nothing in this loop
> adds anything to it.  Because "revs" does get extended in the loop,
> it is understandable if you wanted to clear it before dying, but "if
> you are dying anyway why bother clearing?" is also a valid stance to
> take.

I think I should probably use return here instead of die().

> The same "perhaps want to use the 'retval' with goto 'finish:' pattern?"
> comment applies here, too.

Okay sure could do that.

>> + pathspec_pos = i;
>> +
>> + /*
>> +  * The user ran "git bisect start  ", hence did not
>> +  * explicitly specify the terms, but we are already starting to
>> +  * set references named with the default terms, and won't be able
>> +  * to change afterwards.
>> +  */
>> + must_write_terms |= !!revs.nr;
>> + for (i = 0; i < revs.nr; i++) {
>> + if (bad_seen)
>> + string_list_append(, terms->term_good.buf);
>> + else {
>> + bad_seen = 1;
>> + string_list_append(, terms->term_bad.buf);
>> + }
>> + }
>
> This is certainly different from the original.  We used to do one
> "bisect_write" per element in revs in the loop; we no longer do that
> and instead collect them in states list.  Each element in these two
> lists, i.e. revs.item[i] and states.item[i], corresponds to each
> other.
>
> That explains why the variable is renamed to states.  We haven't
> seen enough to say if this behaviour change is a good idea or not.
>
>> + /*
>> +  * Verify HEAD
>> +  */
>> + head = resolve_ref_unsafe("HEAD", 0, sha1, );
>> + if (!head) {
>> + if (get_sha1("HEAD", sha1)) {
>> + string_list_clear(, 0);
>> + string_list_clear(, 0);
>> + die(_("Bad HEAD - I need a HEAD"));
>> + }
>> + }
>> + if (!is_empty_or_missing_file(git_path_bisect_start())) {
>> + /* Reset to the rev from where we started */
>> + strbuf_read_file(_head, git_path_bisect_start(), 0);
>> + strbuf_trim(_head);
>> + if (!no_checkout) {
>> + struct argv_array argv = ARGV_ARRAY_INIT;
>> + argv_array_pushl(, "checkout", start_head.buf,
>> +  "--", NULL);
>> + if (run_command_v_opt(argv.argv, RUN_GIT_CMD)) {
>> + error(_("checking out '%s' failed. Try 'git "
>> + "bisect start '."),
>> +   start_head.buf);
>> + strbuf_release(_head);
>> + string_list_clear(, 0);
>> + string_list_clear(, 0);
>> + return -1;
>
> The original died here, but you expect the caller to respond to a
> negative return.  I haven't read enough to judge if that is a good
> idea, but doesn't it make sense to do the same throughout the
> function consistently?  I saw a few die()'s 

Re: [PATCH v13 00/14] libify apply and use lib in am, part 3

2016-08-27 Thread Christian Couder
On Sat, Aug 27, 2016 at 8:45 PM, Christian Couder
 wrote:
>
> I will send a diff between this version and v12 as a reply to this
> email.

Here is the diff:

diff --git a/apply.c b/apply.c
index 7e561a4..5cd037d 100644
--- a/apply.c
+++ b/apply.c
@@ -3993,12 +3993,21 @@ static int check_patch_list(struct apply_state
*state, struct patch *patch)
 return err;
 }

+static int read_apply_cache(struct apply_state *state)
+{
+if (state->index_file)
+return read_cache_from(state->index_file);
+else
+return read_cache();
+}
+
 /* This function tries to read the sha1 from the current index */
-static int get_current_sha1(const char *path, unsigned char *sha1)
+static int get_current_sha1(struct apply_state *state, const char *path,
+unsigned char *sha1)
 {
 int pos;

-if (read_cache() < 0)
+if (read_apply_cache(state) < 0)
 return -1;
 pos = cache_name_pos(path, strlen(path));
 if (pos < 0)
@@ -4042,7 +4051,7 @@ static int preimage_sha1_in_gitlink_patch(struct
patch *p, unsigned char sha1[20
 }

 /* Build an index that contains the just the files needed for a 3way merge */
-static int build_fake_ancestor(struct patch *list, const char *filename)
+static int build_fake_ancestor(struct apply_state *state, struct patch *list)
 {
 struct patch *patch;
 struct index_state result = { NULL };
@@ -4071,7 +4080,7 @@ static int build_fake_ancestor(struct patch
*list, const char *filename)
 ; /* ok */
 } else if (!patch->lines_added && !patch->lines_deleted) {
 /* mode-only change: update the current */
-if (get_current_sha1(patch->old_name, sha1))
+if (get_current_sha1(state, patch->old_name, sha1))
 return error("mode change for %s, which is not "
  "in current HEAD", name);
 } else
@@ -4089,12 +4098,13 @@ static int build_fake_ancestor(struct patch
*list, const char *filename)
 }
 }

-hold_lock_file_for_update(, filename, LOCK_DIE_ON_ERROR);
+hold_lock_file_for_update(, state->fake_ancestor, LOCK_DIE_ON_ERROR);
 res = write_locked_index(, , COMMIT_LOCK);
 discard_index();

 if (res)
-return error("Could not write temporary index to %s", filename);
+return error("Could not write temporary index to %s",
+ state->fake_ancestor);

 return 0;
 }
@@ -4683,7 +4693,7 @@ static int apply_patch(struct apply_state *state,
 state->newfd = hold_locked_index(state->lock_file, 1);
 }

-if (state->check_index && read_cache() < 0) {
+if (state->check_index && read_apply_cache(state) < 0) {
 error(_("unable to read index file"));
 res = -128;
 goto end;
@@ -4715,7 +4725,7 @@ static int apply_patch(struct apply_state *state,
 }

 if (state->fake_ancestor &&
-build_fake_ancestor(list, state->fake_ancestor)) {
+build_fake_ancestor(state, list)) {
 res = -128;
 goto end;
 }
--
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 v13 11/14] apply: refactor `git apply` option parsing

2016-08-27 Thread Christian Couder
Parsing `git apply` options can be useful to other commands that
want to call the libified apply functionality, because this way
they can easily pass some options from their own command line to
the libified apply functionality.

This will be used by `git am` in a following patch.

To make this possible, let's refactor the `git apply` option
parsing code into a new libified apply_parse_options() function.

Doing that makes it possible to remove some functions definitions
from "apply.h" and make them static in "apply.c".

Helped-by: Ramsay Jones 
Signed-off-by: Christian Couder 
---
 apply.c | 103 +---
 apply.h |  18 +++---
 builtin/apply.c |  74 ++--
 3 files changed, 97 insertions(+), 98 deletions(-)

diff --git a/apply.c b/apply.c
index bf81b70..2ec2a8a 100644
--- a/apply.c
+++ b/apply.c
@@ -4730,16 +4730,16 @@ static int apply_patch(struct apply_state *state,
return res;
 }
 
-int apply_option_parse_exclude(const struct option *opt,
-  const char *arg, int unset)
+static int apply_option_parse_exclude(const struct option *opt,
+ const char *arg, int unset)
 {
struct apply_state *state = opt->value;
add_name_limit(state, arg, 1);
return 0;
 }
 
-int apply_option_parse_include(const struct option *opt,
-  const char *arg, int unset)
+static int apply_option_parse_include(const struct option *opt,
+ const char *arg, int unset)
 {
struct apply_state *state = opt->value;
add_name_limit(state, arg, 0);
@@ -4747,9 +4747,9 @@ int apply_option_parse_include(const struct option *opt,
return 0;
 }
 
-int apply_option_parse_p(const struct option *opt,
-const char *arg,
-int unset)
+static int apply_option_parse_p(const struct option *opt,
+   const char *arg,
+   int unset)
 {
struct apply_state *state = opt->value;
state->p_value = atoi(arg);
@@ -4757,8 +4757,8 @@ int apply_option_parse_p(const struct option *opt,
return 0;
 }
 
-int apply_option_parse_space_change(const struct option *opt,
-   const char *arg, int unset)
+static int apply_option_parse_space_change(const struct option *opt,
+  const char *arg, int unset)
 {
struct apply_state *state = opt->value;
if (unset)
@@ -4768,8 +4768,8 @@ int apply_option_parse_space_change(const struct option 
*opt,
return 0;
 }
 
-int apply_option_parse_whitespace(const struct option *opt,
- const char *arg, int unset)
+static int apply_option_parse_whitespace(const struct option *opt,
+const char *arg, int unset)
 {
struct apply_state *state = opt->value;
state->whitespace_option = arg;
@@ -4778,8 +4778,8 @@ int apply_option_parse_whitespace(const struct option 
*opt,
return 0;
 }
 
-int apply_option_parse_directory(const struct option *opt,
-const char *arg, int unset)
+static int apply_option_parse_directory(const struct option *opt,
+   const char *arg, int unset)
 {
struct apply_state *state = opt->value;
strbuf_reset(>root);
@@ -4893,3 +4893,80 @@ int apply_all_patches(struct apply_state *state,
return res;
return (res == -1 ? 1 : 128);
 }
+
+int apply_parse_options(int argc, const char **argv,
+   struct apply_state *state,
+   int *force_apply, int *options,
+   const char * const *apply_usage)
+{
+   struct option builtin_apply_options[] = {
+   { OPTION_CALLBACK, 0, "exclude", state, N_("path"),
+   N_("don't apply changes matching the given path"),
+   0, apply_option_parse_exclude },
+   { OPTION_CALLBACK, 0, "include", state, N_("path"),
+   N_("apply changes matching the given path"),
+   0, apply_option_parse_include },
+   { OPTION_CALLBACK, 'p', NULL, state, N_("num"),
+   N_("remove  leading slashes from traditional diff 
paths"),
+   0, apply_option_parse_p },
+   OPT_BOOL(0, "no-add", >no_add,
+   N_("ignore additions made by the patch")),
+   OPT_BOOL(0, "stat", >diffstat,
+   N_("instead of applying the patch, output diffstat for 
the input")),
+   OPT_NOOP_NOARG(0, "allow-binary-replacement"),
+   OPT_NOOP_NOARG(0, "binary"),
+   OPT_BOOL(0, "numstat", >numstat,
+  

[PATCH v13 00/14] libify apply and use lib in am, part 3

2016-08-27 Thread Christian Couder
Goal


This is a patch series about libifying `git apply` functionality, and
using this libified functionality in `git am`, so that no 'git apply'
process is spawn anymore. This makes `git am` significantly faster, so
`git rebase`, when it uses the am backend, is also significantly
faster.

Previous discussions and patches series
~~~

This has initially been discussed in the following thread:

  http://thread.gmane.org/gmane.comp.version-control.git/287236/

Then the following patch series were sent:

RFC: http://thread.gmane.org/gmane.comp.version-control.git/288489/
v1: http://thread.gmane.org/gmane.comp.version-control.git/292324/
v2: http://thread.gmane.org/gmane.comp.version-control.git/294248/
v3: http://thread.gmane.org/gmane.comp.version-control.git/295429/
v4: http://thread.gmane.org/gmane.comp.version-control.git/296350/
v5: http://thread.gmane.org/gmane.comp.version-control.git/296490/
v6: http://thread.gmane.org/gmane.comp.version-control.git/297024/
v7: http://thread.gmane.org/gmane.comp.version-control.git/297193/
v8: 
https://public-inbox.org/git/20160627182429.31550-1-chriscool%40tuxfamily.org/
v9: 
https://public-inbox.org/git/20160730172509.22939-1-chriscool%40tuxfamily.org/
v10: 
https://public-inbox.org/git/20160808210337.5038-1-chriscool%40tuxfamily.org/
v11: 
https://public-inbox.org/git/20160811085229.19017-1-chriscool%40tuxfamily.org/
v12: https://public-inbox.org/git/20160811184501.384-1-chrisc...@tuxfamily.org/

Highlevel view of the patches in the series
~~~

This is "part 3" of the full patch series. I am resending only the
last 14 patches of the full series as "part 3", because I don't want
to resend the first 27 patches of v10 nearly as is.

(So "part 2" is made of patch 01/40 from v11 and patches from 02/40 to
27/40 from v10. And "part 1" has been in "master" for some time now.)

  - Patch 01/14 to 04/14 were v1, v2, v6, v7, v8, v9, v10, v11 and v12.

They haven't changed since v12.

They finish libifying the apply functionality that was in
builtin/apply.c and move it into apply.{c,h}, but the libified
functionality is not yet used in `git am`.

  - Patch 05/14 was in v6, v7, v8, v9, v10 and v12 and hasn't changed.

It replaces some calls to error() with calls to error_errno().

  - Patches 06/14 to 10/14 were in v2, v6, v7, v8, v9 and v10.

They haven't changed since v10.

They implement a way to make the libified apply code silent by
changing the bool `apply_verbosely` into a tristate enum called
`apply_verbosity`, that can be one of `verbosity_verbose`,
`verbosity_normal` or `verbosity_silent`.

This is because "git am", since it was a shell script, has been
silencing the apply functionality by redirecting file descriptors to
/dev/null, but this is not acceptable in C.

  - Patch 11/14 was in v9, v10 and v12, and hasn't changed.

It refactors `git apply` option parsing to make it possible for `git
am` to easily pass some command line options to the libified applied
code.

  - Patch 12/14 is new.

It is a refactoring to prepare for some new changes in patch 13/14.

  - Patch 13/14 was in v12.

It replaces patch 33/40 in v10 (environment: add set_index_file())
that was a hack to make it possible for `git am` to use the libified
apply functionality on a different index file.

For this purpose in this patch we add a "const char *index_file" into
"struct apply_state", and when "index_file" is set, we use
hold_lock_file_for_update(), passing it "state->index_file", instead
of using hold_locked_index(), as it is not possible to pass an index
filename in hold_locked_index().

This patch was changed in this version to also use
"read_cache_from(state->index_file)" instead of "read_cache()" when
state->index_file is set.

  - Patch 14/14 was in v1, v2, v6, v7, v8, v9, v10 and v12, and hasn't
changed since v12.

This patch makes `git am` use the libified functionality.

General comments


Now this patch series is shorter than previously. Hopefully also the
early part of this series until 05/14 or 11/14 will be ready soon to
be moved to next and master, and I may only need to resend the last 3
patches if anything.

I will send a diff between this version and v12 as a reply to this
email.

The benefits are not just related to not creating new processes. When
`git am` launched a `git apply` process, this new process had to read
the index from disk. Then after the `git apply`process had terminated,
`git am` dropped its index and read the index from disk to get the
index that had been modified by the `git apply`process. This was
inefficient and also prevented the split-index mechanism to provide
many performance benefits.

By the way, current work is ongoing to make it possible to use
split-index more easily by adding a config variable, see:

https://public-inbox.org/git/20160711172254.13439-1-chriscool%40tuxfamily.org/

Using an earlier version of this series as rebase material, Duy

[PATCH v13 01/14] builtin/apply: rename option parsing functions

2016-08-27 Thread Christian Couder
As these functions are going to be part of the libified
apply API, let's give them a name that is more specific
to the apply API.

Signed-off-by: Christian Couder 
---
 builtin/apply.c | 40 
 1 file changed, 20 insertions(+), 20 deletions(-)

diff --git a/builtin/apply.c b/builtin/apply.c
index ad403f8..429fe44 100644
--- a/builtin/apply.c
+++ b/builtin/apply.c
@@ -4588,16 +4588,16 @@ static int apply_patch(struct apply_state *state,
return res;
 }
 
-static int option_parse_exclude(const struct option *opt,
-   const char *arg, int unset)
+static int apply_option_parse_exclude(const struct option *opt,
+ const char *arg, int unset)
 {
struct apply_state *state = opt->value;
add_name_limit(state, arg, 1);
return 0;
 }
 
-static int option_parse_include(const struct option *opt,
-   const char *arg, int unset)
+static int apply_option_parse_include(const struct option *opt,
+ const char *arg, int unset)
 {
struct apply_state *state = opt->value;
add_name_limit(state, arg, 0);
@@ -4605,9 +4605,9 @@ static int option_parse_include(const struct option *opt,
return 0;
 }
 
-static int option_parse_p(const struct option *opt,
- const char *arg,
- int unset)
+static int apply_option_parse_p(const struct option *opt,
+   const char *arg,
+   int unset)
 {
struct apply_state *state = opt->value;
state->p_value = atoi(arg);
@@ -4615,8 +4615,8 @@ static int option_parse_p(const struct option *opt,
return 0;
 }
 
-static int option_parse_space_change(const struct option *opt,
-const char *arg, int unset)
+static int apply_option_parse_space_change(const struct option *opt,
+  const char *arg, int unset)
 {
struct apply_state *state = opt->value;
if (unset)
@@ -4626,8 +4626,8 @@ static int option_parse_space_change(const struct option 
*opt,
return 0;
 }
 
-static int option_parse_whitespace(const struct option *opt,
-  const char *arg, int unset)
+static int apply_option_parse_whitespace(const struct option *opt,
+const char *arg, int unset)
 {
struct apply_state *state = opt->value;
state->whitespace_option = arg;
@@ -4636,8 +4636,8 @@ static int option_parse_whitespace(const struct option 
*opt,
return 0;
 }
 
-static int option_parse_directory(const struct option *opt,
- const char *arg, int unset)
+static int apply_option_parse_directory(const struct option *opt,
+   const char *arg, int unset)
 {
struct apply_state *state = opt->value;
strbuf_reset(>root);
@@ -4755,13 +4755,13 @@ int cmd_apply(int argc, const char **argv, const char 
*prefix)
struct option builtin_apply_options[] = {
{ OPTION_CALLBACK, 0, "exclude", , N_("path"),
N_("don't apply changes matching the given path"),
-   0, option_parse_exclude },
+   0, apply_option_parse_exclude },
{ OPTION_CALLBACK, 0, "include", , N_("path"),
N_("apply changes matching the given path"),
-   0, option_parse_include },
+   0, apply_option_parse_include },
{ OPTION_CALLBACK, 'p', NULL, , N_("num"),
N_("remove  leading slashes from traditional diff 
paths"),
-   0, option_parse_p },
+   0, apply_option_parse_p },
OPT_BOOL(0, "no-add", _add,
N_("ignore additions made by the patch")),
OPT_BOOL(0, "stat", ,
@@ -4793,13 +4793,13 @@ int cmd_apply(int argc, const char **argv, const char 
*prefix)
N_("ensure at least  lines of context 
match")),
{ OPTION_CALLBACK, 0, "whitespace", , N_("action"),
N_("detect new or modified lines that have whitespace 
errors"),
-   0, option_parse_whitespace },
+   0, apply_option_parse_whitespace },
{ OPTION_CALLBACK, 0, "ignore-space-change", , NULL,
N_("ignore changes in whitespace when finding context"),
-   PARSE_OPT_NOARG, option_parse_space_change },
+   PARSE_OPT_NOARG, apply_option_parse_space_change },
{ OPTION_CALLBACK, 0, "ignore-whitespace", , NULL,
N_("ignore changes in whitespace when finding context"),
-   PARSE_OPT_NOARG, 

[PATCH v13 05/14] apply: use error_errno() where possible

2016-08-27 Thread Christian Couder
To avoid possible mistakes and to uniformly show the errno
related messages, let's use error_errno() where possible.

Signed-off-by: Christian Couder 
---
 apply.c | 18 +-
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/apply.c b/apply.c
index c0cb3f5..41a33d3 100644
--- a/apply.c
+++ b/apply.c
@@ -3497,7 +3497,7 @@ static int load_current(struct apply_state *state,
ce = active_cache[pos];
if (lstat(name, )) {
if (errno != ENOENT)
-   return error(_("%s: %s"), name, strerror(errno));
+   return error_errno("%s", name);
if (checkout_target(_index, ce, ))
return -1;
}
@@ -3647,7 +3647,7 @@ static int check_preimage(struct apply_state *state,
} else if (!state->cached) {
stat_ret = lstat(old_name, st);
if (stat_ret && errno != ENOENT)
-   return error(_("%s: %s"), old_name, strerror(errno));
+   return error_errno("%s", old_name);
}
 
if (state->check_index && !previous) {
@@ -3669,7 +3669,7 @@ static int check_preimage(struct apply_state *state,
} else if (stat_ret < 0) {
if (patch->is_new < 0)
goto is_new;
-   return error(_("%s: %s"), old_name, strerror(errno));
+   return error_errno("%s", old_name);
}
 
if (!state->cached && !previous)
@@ -3728,7 +3728,7 @@ static int check_to_create(struct apply_state *state,
 
return EXISTS_IN_WORKTREE;
} else if ((errno != ENOENT) && (errno != ENOTDIR)) {
-   return error("%s: %s", new_name, strerror(errno));
+   return error_errno("%s", new_name);
}
return 0;
 }
@@ -4247,9 +4247,9 @@ static int add_index_file(struct apply_state *state,
if (!state->cached) {
if (lstat(path, ) < 0) {
free(ce);
-   return error(_("unable to stat newly "
-  "created file '%s': %s"),
-path, strerror(errno));
+   return error_errno(_("unable to stat newly "
+"created file '%s'"),
+  path);
}
fill_stat_cache_info(ce, );
}
@@ -4306,7 +4306,7 @@ static int try_create_file(const char *path, unsigned int 
mode, const char *buf,
strbuf_release();
 
if (close(fd) < 0 && !res)
-   return error(_("closing file '%s': %s"), path, strerror(errno));
+   return error_errno(_("closing file '%s'"), path);
 
return res ? -1 : 0;
 }
@@ -4503,7 +4503,7 @@ static int write_out_one_reject(struct apply_state 
*state, struct patch *patch)
 
rej = fopen(namebuf, "w");
if (!rej)
-   return error(_("cannot open %s: %s"), namebuf, strerror(errno));
+   return error_errno(_("cannot open %s"), namebuf);
 
/* Normal git tools never deal with .rej, so do not pretend
 * this is a git patch by saying --git or giving extended
-- 
2.9.2.770.g14ff7d2

--
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 v13 06/14] apply: make it possible to silently apply

2016-08-27 Thread Christian Couder
This changes 'int apply_verbosely' into 'enum apply_verbosity', and
changes the possible values of the variable from a bool to
a tristate.

The previous 'false' state is changed into 'verbosity_normal'.
The previous 'true' state is changed into 'verbosity_verbose'.

The new added state is 'verbosity_silent'. It should prevent
anything to be printed on both stderr and stdout.

This is needed because `git am` wants to first call apply
functionality silently, if it can then fall back on 3-way merge
in case of error.

Printing on stdout, and calls to warning() or error() are not
taken care of in this patch, as that will be done in following
patches.

Signed-off-by: Christian Couder 
---
 apply.c | 62 +
 apply.h |  8 +++-
 builtin/apply.c |  2 +-
 3 files changed, 48 insertions(+), 24 deletions(-)

diff --git a/apply.c b/apply.c
index 41a33d3..df85cbc 100644
--- a/apply.c
+++ b/apply.c
@@ -125,8 +125,11 @@ int check_apply_state(struct apply_state *state, int 
force_apply)
return error(_("--3way outside a repository"));
state->check_index = 1;
}
-   if (state->apply_with_reject)
-   state->apply = state->apply_verbosely = 1;
+   if (state->apply_with_reject) {
+   state->apply = 1;
+   if (state->apply_verbosity == verbosity_normal)
+   state->apply_verbosity = verbosity_verbose;
+   }
if (!force_apply && (state->diffstat || state->numstat || 
state->summary || state->check || state->fake_ancestor))
state->apply = 0;
if (state->check_index && is_not_gitdir)
@@ -1620,8 +1623,9 @@ static void record_ws_error(struct apply_state *state,
return;
 
err = whitespace_error_string(result);
-   fprintf(stderr, "%s:%d: %s.\n%.*s\n",
-   state->patch_input_file, linenr, err, len, line);
+   if (state->apply_verbosity > verbosity_silent)
+   fprintf(stderr, "%s:%d: %s.\n%.*s\n",
+   state->patch_input_file, linenr, err, len, line);
free(err);
 }
 
@@ -1816,7 +1820,7 @@ static int parse_single_patch(struct apply_state *state,
return error(_("new file %s depends on old contents"), 
patch->new_name);
if (0 < patch->is_delete && newlines)
return error(_("deleted file %s still has contents"), 
patch->old_name);
-   if (!patch->is_delete && !newlines && context)
+   if (!patch->is_delete && !newlines && context && state->apply_verbosity 
> verbosity_silent)
fprintf_ln(stderr,
   _("** warning: "
 "file %s becomes empty but is not deleted"),
@@ -2911,7 +2915,7 @@ static int apply_one_fragment(struct apply_state *state,
/* Ignore it, we already handled it */
break;
default:
-   if (state->apply_verbosely)
+   if (state->apply_verbosity > verbosity_normal)
error(_("invalid start of line: '%c'"), first);
applied_pos = -1;
goto out;
@@ -3026,7 +3030,7 @@ static int apply_one_fragment(struct apply_state *state,
state->apply = 0;
}
 
-   if (state->apply_verbosely && applied_pos != pos) {
+   if (state->apply_verbosity > verbosity_normal && applied_pos != 
pos) {
int offset = applied_pos - pos;
if (state->apply_in_reverse)
offset = 0 - offset;
@@ -3041,14 +3045,14 @@ static int apply_one_fragment(struct apply_state *state,
 * Warn if it was necessary to reduce the number
 * of context lines.
 */
-   if ((leading != frag->leading) ||
-   (trailing != frag->trailing))
+   if ((leading != frag->leading ||
+trailing != frag->trailing) && state->apply_verbosity > 
verbosity_silent)
fprintf_ln(stderr, _("Context reduced to (%ld/%ld)"
 " to apply fragment at %d"),
   leading, trailing, applied_pos+1);
update_image(state, img, applied_pos, , );
} else {
-   if (state->apply_verbosely)
+   if (state->apply_verbosity > verbosity_normal)
error(_("while searching for:\n%.*s"),
  (int)(old - oldlines), oldlines);
}
@@ -3539,7 +3543,8 @@ static int try_threeway(struct apply_state *state,
 read_blob_object(, pre_sha1, patch->old_mode))
return error("repository lacks the necessary blob to fall back 
on 3-way merge.");
 
-   

[PATCH v13 14/14] builtin/am: use apply API in run_apply()

2016-08-27 Thread Christian Couder
This replaces run_apply() implementation with a new one that
uses the apply API that has been previously prepared in
apply.c and apply.h.

This shoud improve performance a lot in certain cases.

As the previous implementation was creating a new `git apply`
process to apply each patch, it could be slow on systems like
Windows where it is costly to create new processes.

Also the new `git apply` process had to read the index from
disk, and when the process was done the calling process
discarded its own index and read back from disk the new
index that had been created by the `git apply` process.

This could be very inefficient with big repositories that
have big index files, especially when the system decided
that it was a good idea to run the `git apply` processes on
a different processor core.

Also eliminating index reads enables further performance
improvements by using:

`git update-index --split-index`

For example here is a benchmark of a multi hundred commit
rebase on the Linux kernel on a Debian laptop with SSD:

command: git rebase --onto 1993b17 52bef0c 29dde7c

Vanilla "next" without split index:1m54.953s
Vanilla "next" with split index:   1m22.476s
This series on top of "next" without split index:  1m12.034s
This series on top of "next" with split index: 0m15.678s

(using branch "next" from mid April 2016.)

Benchmarked-by: Ævar Arnfjörð Bjarmason 
Signed-off-by: Christian Couder 
---
 builtin/am.c | 65 
 1 file changed, 43 insertions(+), 22 deletions(-)

diff --git a/builtin/am.c b/builtin/am.c
index 739b34d..0e5d384 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -28,6 +28,7 @@
 #include "rerere.h"
 #include "prompt.h"
 #include "mailinfo.h"
+#include "apply.h"
 
 /**
  * Returns 1 if the file is empty or does not exist, 0 otherwise.
@@ -1522,39 +1523,59 @@ static int parse_mail_rebase(struct am_state *state, 
const char *mail)
  */
 static int run_apply(const struct am_state *state, const char *index_file)
 {
-   struct child_process cp = CHILD_PROCESS_INIT;
-
-   cp.git_cmd = 1;
-
-   if (index_file)
-   argv_array_pushf(_array, "GIT_INDEX_FILE=%s", 
index_file);
+   struct argv_array apply_paths = ARGV_ARRAY_INIT;
+   struct argv_array apply_opts = ARGV_ARRAY_INIT;
+   struct apply_state apply_state;
+   int res, opts_left;
+   static struct lock_file lock_file;
+   int force_apply = 0;
+   int options = 0;
+
+   if (init_apply_state(_state, NULL, _file))
+   die("BUG: init_apply_state() failed");
+
+   argv_array_push(_opts, "apply");
+   argv_array_pushv(_opts, state->git_apply_opts.argv);
+
+   opts_left = apply_parse_options(apply_opts.argc, apply_opts.argv,
+   _state, _apply, ,
+   NULL);
+
+   if (opts_left != 0)
+   die("unknown option passed thru to git apply");
+
+   if (index_file) {
+   apply_state.index_file = index_file;
+   apply_state.cached = 1;
+   } else
+   apply_state.check_index = 1;
 
/*
 * If we are allowed to fall back on 3-way merge, don't give false
 * errors during the initial attempt.
 */
-   if (state->threeway && !index_file) {
-   cp.no_stdout = 1;
-   cp.no_stderr = 1;
-   }
+   if (state->threeway && !index_file)
+   apply_state.apply_verbosity = verbosity_silent;
 
-   argv_array_push(, "apply");
+   if (check_apply_state(_state, force_apply))
+   die("BUG: check_apply_state() failed");
 
-   argv_array_pushv(, state->git_apply_opts.argv);
+   argv_array_push(_paths, am_path(state, "patch"));
 
-   if (index_file)
-   argv_array_push(, "--cached");
-   else
-   argv_array_push(, "--index");
+   res = apply_all_patches(_state, apply_paths.argc, 
apply_paths.argv, options);
 
-   argv_array_push(, am_path(state, "patch"));
+   argv_array_clear(_paths);
+   argv_array_clear(_opts);
+   clear_apply_state(_state);
 
-   if (run_command())
-   return -1;
+   if (res)
+   return res;
 
-   /* Reload index as git-apply will have modified it. */
-   discard_cache();
-   read_cache_from(index_file ? index_file : get_index_file());
+   if (index_file) {
+   /* Reload index as apply_all_patches() will have modified it. */
+   discard_cache();
+   read_cache_from(index_file);
+   }
 
return 0;
 }
-- 
2.9.2.770.g14ff7d2

--
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 v13 13/14] apply: learn to use a different index file

2016-08-27 Thread Christian Couder
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 | 27 +--
 apply.h |  1 +
 2 files changed, 22 insertions(+), 6 deletions(-)

diff --git a/apply.c b/apply.c
index 276e4af..5cd037d 100644
--- a/apply.c
+++ b/apply.c
@@ -3993,12 +3993,21 @@ static int check_patch_list(struct apply_state *state, 
struct patch *patch)
return err;
 }
 
+static int read_apply_cache(struct apply_state *state)
+{
+   if (state->index_file)
+   return read_cache_from(state->index_file);
+   else
+   return read_cache();
+}
+
 /* This function tries to read the sha1 from the current index */
-static int get_current_sha1(const char *path, unsigned char *sha1)
+static int get_current_sha1(struct apply_state *state, const char *path,
+   unsigned char *sha1)
 {
int pos;
 
-   if (read_cache() < 0)
+   if (read_apply_cache(state) < 0)
return -1;
pos = cache_name_pos(path, strlen(path));
if (pos < 0)
@@ -4071,7 +4080,7 @@ static int build_fake_ancestor(struct apply_state *state, 
struct patch *list)
; /* ok */
} else if (!patch->lines_added && !patch->lines_deleted) {
/* mode-only change: update the current */
-   if (get_current_sha1(patch->old_name, sha1))
+   if (get_current_sha1(state, patch->old_name, sha1))
return error("mode change for %s, which is not "
 "in current HEAD", name);
} else
@@ -4675,10 +4684,16 @@ 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) {
+   if (state->check_index && read_apply_cache(state) < 0) {
error(_("unable to read index file"));
res = -128;
goto end;
diff --git a/apply.h b/apply.h
index e2b89e8..1ba4f8d 100644
--- a/apply.h
+++ b/apply.h
@@ -63,6 +63,7 @@ struct apply_state {
int unsafe_paths;
 
/* Other non boolean parameters */
+   const char *index_file;
enum apply_verbosity apply_verbosity;
const char *fake_ancestor;
const char *patch_input_file;
-- 
2.9.2.770.g14ff7d2

--
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 v13 12/14] apply: pass apply state to build_fake_ancestor()

2016-08-27 Thread Christian Couder
To libify git apply functionality, we will need to read from a
different index file in get_current_sha1(). This index file will be
stored in "struct apply_state", so let's pass the state to
build_fake_ancestor() which will later pass it to get_current_sha1().

Signed-off-by: Christian Couder 
---
 apply.c | 9 +
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/apply.c b/apply.c
index 2ec2a8a..276e4af 100644
--- a/apply.c
+++ b/apply.c
@@ -4042,7 +4042,7 @@ static int preimage_sha1_in_gitlink_patch(struct patch 
*p, unsigned char sha1[20
 }
 
 /* Build an index that contains the just the files needed for a 3way merge */
-static int build_fake_ancestor(struct patch *list, const char *filename)
+static int build_fake_ancestor(struct apply_state *state, struct patch *list)
 {
struct patch *patch;
struct index_state result = { NULL };
@@ -4089,12 +4089,13 @@ static int build_fake_ancestor(struct patch *list, 
const char *filename)
}
}
 
-   hold_lock_file_for_update(, filename, LOCK_DIE_ON_ERROR);
+   hold_lock_file_for_update(, state->fake_ancestor, 
LOCK_DIE_ON_ERROR);
res = write_locked_index(, , COMMIT_LOCK);
discard_index();
 
if (res)
-   return error("Could not write temporary index to %s", filename);
+   return error("Could not write temporary index to %s",
+state->fake_ancestor);
 
return 0;
 }
@@ -4709,7 +4710,7 @@ static int apply_patch(struct apply_state *state,
}
 
if (state->fake_ancestor &&
-   build_fake_ancestor(list, state->fake_ancestor)) {
+   build_fake_ancestor(state, list)) {
res = -128;
goto end;
}
-- 
2.9.2.770.g14ff7d2

--
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 v13 09/14] usage: add get_error_routine() and get_warn_routine()

2016-08-27 Thread Christian Couder
Let's make it possible to get the current error_routine and warn_routine,
so that we can store them before using set_error_routine() or
set_warn_routine() to use new ones.

This way we will be able put back the original routines, when we are done
with using new ones.

Signed-off-by: Christian Couder 
---
 git-compat-util.h |  2 ++
 usage.c   | 10 ++
 2 files changed, 12 insertions(+)

diff --git a/git-compat-util.h b/git-compat-util.h
index c7a51f8..de04df1 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -440,7 +440,9 @@ static inline int const_error(void)
 
 extern void set_die_routine(NORETURN_PTR void (*routine)(const char *err, 
va_list params));
 extern void set_error_routine(void (*routine)(const char *err, va_list 
params));
+extern void (*get_error_routine(void))(const char *err, va_list params);
 extern void set_warn_routine(void (*routine)(const char *warn, va_list 
params));
+extern void (*get_warn_routine(void))(const char *warn, va_list params);
 extern void set_die_is_recursing_routine(int (*routine)(void));
 extern void set_error_handle(FILE *);
 
diff --git a/usage.c b/usage.c
index 67e5526..2fd3045 100644
--- a/usage.c
+++ b/usage.c
@@ -70,11 +70,21 @@ void set_error_routine(void (*routine)(const char *err, 
va_list params))
error_routine = routine;
 }
 
+void (*get_error_routine(void))(const char *err, va_list params)
+{
+   return error_routine;
+}
+
 void set_warn_routine(void (*routine)(const char *warn, va_list params))
 {
warn_routine = routine;
 }
 
+void (*get_warn_routine(void))(const char *warn, va_list params)
+{
+   return warn_routine;
+}
+
 void set_die_is_recursing_routine(int (*routine)(void))
 {
die_is_recursing = routine;
-- 
2.9.2.770.g14ff7d2

--
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 v13 04/14] apply: make some parsing functions static again

2016-08-27 Thread Christian Couder
Some parsing functions that were used in both "apply.c" and
"builtin/apply.c" are now only used in the former, so they
can be made static to "apply.c".

Signed-off-by: Christian Couder 
---
 apply.c | 6 +++---
 apply.h | 5 -
 2 files changed, 3 insertions(+), 8 deletions(-)

diff --git a/apply.c b/apply.c
index 7b96130..c0cb3f5 100644
--- a/apply.c
+++ b/apply.c
@@ -27,7 +27,7 @@ static void git_apply_config(void)
git_config(git_default_config, NULL);
 }
 
-int parse_whitespace_option(struct apply_state *state, const char *option)
+static int parse_whitespace_option(struct apply_state *state, const char 
*option)
 {
if (!option) {
state->ws_error_action = warn_on_ws_error;
@@ -57,8 +57,8 @@ int parse_whitespace_option(struct apply_state *state, const 
char *option)
return error(_("unrecognized whitespace option '%s'"), option);
 }
 
-int parse_ignorewhitespace_option(struct apply_state *state,
- const char *option)
+static int parse_ignorewhitespace_option(struct apply_state *state,
+const char *option)
 {
if (!option || !strcmp(option, "no") ||
!strcmp(option, "false") || !strcmp(option, "never") ||
diff --git a/apply.h b/apply.h
index 5ec022c..df44b51 100644
--- a/apply.h
+++ b/apply.h
@@ -97,11 +97,6 @@ struct apply_state {
int applied_after_fixing_ws;
 };
 
-extern int parse_whitespace_option(struct apply_state *state,
-  const char *option);
-extern int parse_ignorewhitespace_option(struct apply_state *state,
-const char *option);
-
 extern int apply_option_parse_exclude(const struct option *opt,
  const char *arg, int unset);
 extern int apply_option_parse_include(const struct option *opt,
-- 
2.9.2.770.g14ff7d2

--
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 v13 02/14] apply: rename and move opt constants to apply.h

2016-08-27 Thread Christian Couder
The constants for the "inaccurate-eof" and the "recount" options will
be used in both "apply.c" and "builtin/apply.c", so they need to go
into "apply.h", and therefore they need a name that is more specific
to the API they belong to.

Signed-off-by: Christian Couder 
---
 apply.h |  3 +++
 builtin/apply.c | 11 ---
 2 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/apply.h b/apply.h
index 53f09b5..ca1dcee 100644
--- a/apply.h
+++ b/apply.h
@@ -108,4 +108,7 @@ extern int init_apply_state(struct apply_state *state,
 extern void clear_apply_state(struct apply_state *state);
 extern int check_apply_state(struct apply_state *state, int force_apply);
 
+#define APPLY_OPT_INACCURATE_EOF   (1<<0)
+#define APPLY_OPT_RECOUNT  (1<<1)
+
 #endif
diff --git a/builtin/apply.c b/builtin/apply.c
index 429fe44..9c396bb 100644
--- a/builtin/apply.c
+++ b/builtin/apply.c
@@ -4463,9 +4463,6 @@ static int write_out_results(struct apply_state *state, 
struct patch *list)
 
 static struct lock_file lock_file;
 
-#define INACCURATE_EOF (1<<0)
-#define RECOUNT(1<<1)
-
 /*
  * Try to apply a patch.
  *
@@ -4495,8 +4492,8 @@ static int apply_patch(struct apply_state *state,
int nr;
 
patch = xcalloc(1, sizeof(*patch));
-   patch->inaccurate_eof = !!(options & INACCURATE_EOF);
-   patch->recount =  !!(options & RECOUNT);
+   patch->inaccurate_eof = !!(options & APPLY_OPT_INACCURATE_EOF);
+   patch->recount =  !!(options & APPLY_OPT_RECOUNT);
nr = parse_chunk(state, buf.buf + offset, buf.len - offset, 
patch);
if (nr < 0) {
free_patch(patch);
@@ -4811,10 +4808,10 @@ int cmd_apply(int argc, const char **argv, const char 
*prefix)
OPT__VERBOSE(_verbosely, N_("be verbose")),
OPT_BIT(0, "inaccurate-eof", ,
N_("tolerate incorrectly detected missing new-line at 
the end of file"),
-   INACCURATE_EOF),
+   APPLY_OPT_INACCURATE_EOF),
OPT_BIT(0, "recount", ,
N_("do not trust the line counts in the hunk headers"),
-   RECOUNT),
+   APPLY_OPT_RECOUNT),
{ OPTION_CALLBACK, 0, "directory", , N_("root"),
N_("prepend  to all filenames"),
0, apply_option_parse_directory },
-- 
2.9.2.770.g14ff7d2

--
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 v13 08/14] usage: add set_warn_routine()

2016-08-27 Thread Christian Couder
There are already set_die_routine() and set_error_routine(),
so let's add set_warn_routine() as this will be needed in a
following commit.

Signed-off-by: Christian Couder 
---
 git-compat-util.h | 1 +
 usage.c   | 5 +
 2 files changed, 6 insertions(+)

diff --git a/git-compat-util.h b/git-compat-util.h
index 590bfdd..c7a51f8 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -440,6 +440,7 @@ static inline int const_error(void)
 
 extern void set_die_routine(NORETURN_PTR void (*routine)(const char *err, 
va_list params));
 extern void set_error_routine(void (*routine)(const char *err, va_list 
params));
+extern void set_warn_routine(void (*routine)(const char *warn, va_list 
params));
 extern void set_die_is_recursing_routine(int (*routine)(void));
 extern void set_error_handle(FILE *);
 
diff --git a/usage.c b/usage.c
index 1dad03f..67e5526 100644
--- a/usage.c
+++ b/usage.c
@@ -70,6 +70,11 @@ void set_error_routine(void (*routine)(const char *err, 
va_list params))
error_routine = routine;
 }
 
+void set_warn_routine(void (*routine)(const char *warn, va_list params))
+{
+   warn_routine = routine;
+}
+
 void set_die_is_recursing_routine(int (*routine)(void))
 {
die_is_recursing = routine;
-- 
2.9.2.770.g14ff7d2

--
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 v13 10/14] apply: change error_routine when silent

2016-08-27 Thread Christian Couder
To avoid printing anything when applying with
`state->apply_verbosity == verbosity_silent`, let's save the
existing warn and error routines before applying, and let's
replace them with a routine that does nothing.

Then after applying, let's restore the saved routines.

Helped-by: Stefan Beller 
Signed-off-by: Christian Couder 
---
 apply.c | 21 -
 apply.h |  8 
 2 files changed, 28 insertions(+), 1 deletion(-)

diff --git a/apply.c b/apply.c
index ddbb0a2..bf81b70 100644
--- a/apply.c
+++ b/apply.c
@@ -112,6 +112,11 @@ void clear_apply_state(struct apply_state *state)
/* >fn_table is cleared at the end of apply_patch() */
 }
 
+static void mute_routine(const char *bla, va_list params)
+{
+   /* do nothing */
+}
+
 int check_apply_state(struct apply_state *state, int force_apply)
 {
int is_not_gitdir = !startup_info->have_repository;
@@ -144,6 +149,13 @@ int check_apply_state(struct apply_state *state, int 
force_apply)
if (!state->lock_file)
return error("BUG: state->lock_file should not be NULL");
 
+   if (state->apply_verbosity <= verbosity_silent) {
+   state->saved_error_routine = get_error_routine();
+   state->saved_warn_routine = get_warn_routine();
+   set_error_routine(mute_routine);
+   set_warn_routine(mute_routine);
+   }
+
return 0;
 }
 
@@ -4864,7 +4876,7 @@ int apply_all_patches(struct apply_state *state,
state->newfd = -1;
}
 
-   return !!errs;
+   res = !!errs;
 
 end:
if (state->newfd >= 0) {
@@ -4872,5 +4884,12 @@ int apply_all_patches(struct apply_state *state,
state->newfd = -1;
}
 
+   if (state->apply_verbosity <= verbosity_silent) {
+   set_error_routine(state->saved_error_routine);
+   set_warn_routine(state->saved_warn_routine);
+   }
+
+   if (res > -1)
+   return res;
return (res == -1 ? 1 : 128);
 }
diff --git a/apply.h b/apply.h
index bd4eb6d..5cde641 100644
--- a/apply.h
+++ b/apply.h
@@ -94,6 +94,14 @@ struct apply_state {
 */
struct string_list fn_table;
 
+   /*
+* This is to save reporting routines before using
+* set_error_routine() or set_warn_routine() to install muting
+* routines when in verbosity_silent mode.
+*/
+   void (*saved_error_routine)(const char *err, va_list params);
+   void (*saved_warn_routine)(const char *warn, va_list params);
+
/* These control whitespace errors */
enum apply_ws_error_action ws_error_action;
enum apply_ws_ignore ws_ignore_action;
-- 
2.9.2.770.g14ff7d2

--
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 v13 07/14] apply: don't print on stdout in verbosity_silent mode

2016-08-27 Thread Christian Couder
When apply_verbosity is set to verbosity_silent nothing should be
printed on both stderr and stdout.

To avoid printing on stdout, we can just skip calling the following
functions:

- stat_patch_list(),
- numstat_patch_list(),
- summary_patch_list().

It is safe to do that because the above functions have no side
effects other than printing:

- stat_patch_list() only computes some local values and then call
show_stats() and print_stat_summary(), those two functions only
compute local values and call printing functions,
- numstat_patch_list() also only computes local values and calls
printing functions,
- summary_patch_list() calls show_file_mode_name(), printf(),
show_rename_copy(), show_mode_change() that are only printing.

Signed-off-by: Christian Couder 
---
 apply.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/apply.c b/apply.c
index df85cbc..ddbb0a2 100644
--- a/apply.c
+++ b/apply.c
@@ -4702,13 +4702,13 @@ static int apply_patch(struct apply_state *state,
goto end;
}
 
-   if (state->diffstat)
+   if (state->diffstat && state->apply_verbosity > verbosity_silent)
stat_patch_list(state, list);
 
-   if (state->numstat)
+   if (state->numstat && state->apply_verbosity > verbosity_silent)
numstat_patch_list(state, list);
 
-   if (state->summary)
+   if (state->summary && state->apply_verbosity > verbosity_silent)
summary_patch_list(list);
 
 end:
-- 
2.9.2.770.g14ff7d2

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


Crash when using git blame on untracked file

2016-08-27 Thread Simon Ruderich
Hello,

I'm seeing the following crash with Git 2.9.3 on Debian sid
(amd64):

$ git init foo
$ cd foo
$ touch x
$ git add x
$ git commit -m test
$ touch x.conf
$ git blame x.conf
segmentation fault

I've tested it on Debian stable's 2.1.4 which works fine:

$ git blame x.conf
fatal: no such path 'x.conf' in HEAD

It requires the blamed file to be present, but in some cases it
works only if the file e.g. "x" is already tracked in Git and the
other file is called "x.conf" (".conf"-suffix). But in an empty
repo it seems to happen always.

Sadly Debian's git has no dbg-package, so the stacktrace is not
very useful:

#0  __strcmp_sse2_unaligned () at 
../sysdeps/x86_64/multiarch/strcmp-sse2-unaligned.S:31
#1  0x0041ad7a in ?? ()
#2  0x00406171 in ?? ()
#3  0x00405321 in ?? ()
#4  0x76f9f700 in __libc_start_main (main=0x4051c0, argc=0x3, 
argv=0x7fffe1a8, init=, fini=, 
rtld_fini=, stack_end=0x7fffe198)
at ../csu/libc-start.c:291
#5  0x004057d9 in ?? ()

Regards
Simon
-- 
+ privacy is necessary
+ using gnupg http://gnupg.org
+ public key id: 0x92FEFDB7E44C32F9


signature.asc
Description: PGP signature


[L10N] Kickoff of translation for Git 2.10.0 round 2

2016-08-27 Thread Jiang Xin
Hi,

Git v2.10.0-rc2 has been released, and let's start new round of git l10n.
This time there are 12 updated messages need lto be translated since last
update:

l10n: git.pot: v2.10.0 round 2 (12 new, 44 removed)

Generate po/git.pot from v2.10.0-rc2 for git v2.10.0 l10n round 2.

Signed-off-by: Jiang Xin 

You can get it from the usual place:

https://github.com/git-l10n/git-po/

As how to update your XX.po and help to translate Git, please see
"Updating a XX.po file" and other sections in “po/README" file.

--
Jiang Xin
--
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: stable as subset of develop

2016-08-27 Thread Jakub Narębski
W dniu 27.08.2016 o 15:17, Brett Porter pisze:
> On 08/27/2016 03:55 AM, Jakub Narębski wrote:
>> W dniu 27.08.2016 o 04:29, Brett Porter pisze:
>>>
>>> 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)

Actually this is almost exactly equivalent (except for the commit
message template) to squash merge (you lose history, that is resulting
commit have only one parent), using 'ours' merge strategy (or rather
'theirs' - which does not exist in Git).

The visualization of history would look like this:


  1<---2<3<4<5<6<7 <--- unstable development branch
  |\  / /
  | \--a Oops - used tag in a generic sense when discussing git -- not helpful.
> Really - would put the hash of the development commit into the log message for
> the stable commit.

Well, this does not change my reasoning in any way.

>>
>> If you are really using topic branches, a better workflow would be
>> to make use of them.  That is, do the development of new features
>> on topic branches, test them in relative isolation, and when deemed
>> ready merge them into development branch, for more testing (including
>> integration testing).
>>
>> Then, those topic branches that are considered stable are merged
>> into stable branch ("trunk").  You can think of it as picking
>> features to have in stable.
> 
> Ok. There are 2 things at play The repository contains code for
> an embedded system with several subsystems (separate executables on
> separate processors). We will be trying to keep testing schemes up to
> date with respect to the progress on the code -- but (beyond
> unit/module tests), the scene will change over time; and, users may
> not be able to run much form their local copies. 

If there is problem with developers running tests (or at least running
them comprehensively, that is for all architectures / subsystems), why
not set up a continuous integration / continuous delivery server, that
would run all the tests after push?

> Second, only 2 of us
> have used git before (and, while trying to get up to speed - I am a
> ways from git guru-dom yet), while everyone else has been using
> visual sourcesafe for many years.

I have not used Visual SourceSafe myself (though I tried to use CVS
in times before Git), but I have heard horror stories about it...

As to mastering Git - I recommend "Pro Git" (free on-line book),
"Git for Teams" (though more about teams than about Git), or my
"Mastering Git" book.

For beginners, there is "Version Control By Example" (free online
version), or "Git: Version Control for Everyone".

> 
> I want others to merge their work into development sooner rather than
> later to minimize their and my problem of untangling conflicts (or -
> you rebased while you were sharing work with Jill and...). And,
> testing on their work may be limited before they push to our main
> repository and some integration can be done.

What the setup looks like?  Is there a central repository where everyone
publish their changes, or is there one person responsible for integrating
changes from each developer public repository?

> 
> I really want to create a linked list of the development branch
> commits that are of interest (their working sets - not the commit
> objects themselves), and using the commit object's pointer to its
> predecessor seems like it could just do the job. (it wouldn't be the
> place to go to for history / useful log 

Re: stable as subset of develop

2016-08-27 Thread Brett Porter

Thank you Jakub for your feedback.

On 08/27/2016 03:55 AM, Jakub Narębski wrote:

W dniu 27.08.2016 o 04:29, Brett Porter pisze:


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)


Oops - used tag in a generic sense when discussing git -- not helpful.
Really - would put the hash of the development commit into the log message for
 the stable commit.



If you are really using topic branches, a better workflow would be
to make use of them.  That is, do the development of new features
on topic branches, test them in relative isolation, and when deemed
ready merge them into development branch, for more testing (including
integration testing).

Then, those topic branches that are considered stable are merged
into stable branch ("trunk").  You can think of it as picking
features to have in stable.


Ok. There are 2 things at play The repository contains code for an embedded
system with several subsystems (separate executables on separate processors). We
will be trying to keep testing schemes up to date with respect to the progress
on the code -- but (beyond unit/module tests), the scene will change over time;
and, users may not be able to run much form their local copies. Second, only 2
of us have used git before (and, while trying to get up to speed - I am a ways
from git guru-dom yet), while everyone else has been using visual sourcesafe for
many years.

I want others to merge their work into development sooner rather than later to
minimize their and my problem of untangling conflicts (or - you rebased while
you were sharing work with Jill and...). And, testing on their work may be 
limited
before they push to our main repository and some integration can be done.

I really want to create a linked list of the development branch commits that are
of interest (their working sets - not the commit objects themselves), and using 
the
commit object's pointer to its predecessor seems like it could just do the job.
(it wouldn't be the place to go to for history / useful log entries)



Take a look at Junio's blog posts about this topic.


Thanks for that. I will.




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


That looks a bit like reimplementation of cherry-picking.


Maybe I've misunderstood cherry-picking, but I've thought it different from the
list view that I've been thinking could help us (with merges and multiple
commits).



Also, I think you would loose the ability to run git-bisect to find
bad commits.


Hmmm - I'm imagining a rather sparse stable, with perhaps time-consuming
integration testing.




This could help with applying successively more intense testing over
time and chase down where problems arose.


Reiterationg: if you are using topic branches, use topic-branch workflow.


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


E-Book zum Thema Geldanlage

2016-08-27 Thread Florian Gerber
Hallo,

unter http://www.meinegeldanlage.com/ biete ich ein kostenloses E-Book zum 
Thema Geldanlage mit umfassenden Informationen zum Download an. Bei den 
Recherchen für das E-Book bin ich unter anderem auf Ihre Webseite gestoßen.

Wären Sie bereit, meine Webseite bzw. das E-Book von Ihrer Webseite aus zu 
verlinken (z. B. von 
http://git.661346.n2.nabble.com/nike-air-max-1-Damen-td7592204.html)?

(Das E-Book kann übrigens ohne Anmeldung oder ähnliche Hürden einfach im 
PDF-Format heruntergeladen werden, ohne weitere Verpflichtungen. Das wird auch 
dauerhaft so bleiben.)

Falls das für Sie interessant ist, kann ich Ihnen gerne einen (eigens 
geschriebenen) Artikel zum Thema zusenden, den Sie auf Ihrer Webseite 
veröffentlichen können.

Wenn Sie möchten, kann ich im Gegenzug Ihre Webseite von einer meiner anderen 
Webseiten verlinken.


Freundliche Grüße,
Florian Gerber
--
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?

2016-08-27 Thread Philip Oakley

From: "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.


If I run them independently, they both find the desired INTERESTED commit, 
hence the expectation that together they will still find that commit as an 
intersection between the two sets.




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.


But it does see F the ultimately desired commit.
Philip@PhilipOakley MINGW32 /usr/src/test (master)

$ git log --oneline --first-parent --merges :/j..

d089efa g

ac811d4 f

1db59e5 c

Then we have:
--ancestry-path
   Limit the displayed commits to those directly on the ancestry chain 
between the "from" and "to" commits in the given commit range.


So one would expect, from a user viewepoint that this is commit selection, 
not internal algorithm implementation, that it would only list G and F, and 
the C would not be displayed.



 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.


The culling order isn't clear. It's easy to expect that --first parent is a 
cull.

The documentation implies (to me) this different computation.


This explains why its happening, but it does feel rather contrary to user 
(rather than developer) expectation. It's the confusion between traversal 
limiting and marking a commit as Interesting (i.e. UNINTERSTING has two 
meanings which may not align when multiple options are given.)


It does seem odd that in the Git world, with its feature branch approach, 
that this hasn't come up before. I know that I haven't even bothered to try 
this 'search', and just use gitk to manually follow the breadcrumbs.


Perhaps a `--first-parent-ancestor` option to determine the places where a 
commit from a feature series was merged in to the mainline would be rather 
helpful.

--
Philip 


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


Re: [PATCH v14 07/27] bisect--helper: `bisect_reset` shell function in C

2016-08-27 Thread Pranit Bauva
Hey Junio,

On Fri, Aug 26, 2016 at 9:59 PM, Junio C Hamano  wrote:
> 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.

I could include this in the commit message. Its not really something
which we would want to test in the function because to the function,
we are not passing the raw arguments. Since we are removing that check
from the function but including it in cmd_bisect__helper(), I will
talk about it in the commit message.

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


[PATCH] make dist: allow using an installed version of git

2016-08-27 Thread Dennis Kaarsemaker
b1de9de2 back in 2005 ensured that we could create a tarball with 'make
dist' even if git wasn't installed yet. These days however, chances are
higher that a git version is available. Add a config.mak knob to allow
people to choose to use the installed version of git to create the
tarball and avoid the overhead of building git-archive.

Signed-off-by: Dennis Kaarsemaker 
---
 Makefile | 14 --
 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/Makefile b/Makefile
index d96ecb7..3dabb75 100644
--- a/Makefile
+++ b/Makefile
@@ -378,6 +378,9 @@ all::
 #
 # to say "export LESS=FRX (and LV=-c) if the environment variable
 # LESS (and LV) is not set, respectively".
+#
+# Define USE_INSTALLED_GIT_ARCHIVE if you don't want to build git-archive as
+# part of 'make dist', but are happy to rely on a git version on you $PATH
 
 GIT-VERSION-FILE: FORCE
@$(SHELL_PATH) ./GIT-VERSION-GEN
@@ -2423,8 +2426,15 @@ quick-install-html:
 ### Maintainer's dist rules
 
 GIT_TARNAME = git-$(GIT_VERSION)
-dist: git-archive$(X) configure
-   ./git-archive --format=tar \
+ifndef USE_INSTALLED_GIT_ARCHIVE
+   GIT_ARCHIVE = ./git-archive$(X)
+   GIT_ARCHIVE_DEP = git-archive$(X)
+else
+   GIT_ARCHIVE = git archive
+   GIT_ARCHIVE_DEP =
+endif
+dist: $(GIT_ARCHIVE_DEP) configure
+   $(GIT_ARCHIVE) --format=tar \
--prefix=$(GIT_TARNAME)/ HEAD^{tree} > $(GIT_TARNAME).tar
@mkdir -p $(GIT_TARNAME)
@cp configure $(GIT_TARNAME)
-- 
2.10.0-rc1-230-g8efea0f
--
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 12/27] bisect--helper: `get_terms` & `bisect_terms` shell function in C

2016-08-27 Thread Pranit Bauva
Hey Junio,

On Thu, Aug 25, 2016 at 11:35 PM, Junio C Hamano  wrote:
> Pranit Bauva  writes:
>
>> +static int bisect_terms(struct bisect_terms *terms, const char **argv, int 
>> argc)
>> +{
>> + int i;
>> +
>> + if (get_terms(terms)) {
>> + fprintf(stderr, _("no terms defined\n"));
>> + return -1;
>> + }
>> + if (argc == 0) {
>> + printf(_("Your current terms are %s for the old state\nand "
>> +"%s for the new state.\n"), terms->term_good.buf,
>> +terms->term_bad.buf);
>> + return 0;
>> + }
>> +
>> + for (i = 0; i < argc; i++) {
>> + if (!strcmp(argv[i], "--term-good"))
>> + printf("%s\n", terms->term_good.buf);
>> + else if (!strcmp(argv[i], "--term-bad"))
>> + printf("%s\n", terms->term_bad.buf);
>> + else
>> + printf(_("invalid argument %s for 'git bisect "
>> +   "terms'.\nSupported options are: "
>> +   "--term-good|--term-old and "
>> +   "--term-bad|--term-new."), argv[i]);
>> + }
>
> The original took only one and gave one answer (and errored out when
> the user asked for more), but this one loops.  I can see either way
> is OK and do not think of a good reason to favor one over the other;
> unless there is a strong reason why you need this extended behaviour
> that allows users to ask multiple questions, I'd say we should keep
> the original behaviour.

True! I can just use return error() instead of printf. Also I noticed
that this is printing to stdout while the original printed it to
stderr. Thanks!

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 v14 11/27] bisect--helper: `bisect_next_check` & bisect_voc shell function in C

2016-08-27 Thread Pranit Bauva
Hey Junio,

On Thu, Aug 25, 2016 at 4:10 AM, Junio C Hamano  wrote:
> Pranit Bauva  writes:
>
>> +static int mark_good(const char *refname, const struct object_id *oid,
>> +  int flag, void *cb_data)
>> +{
>> + int *m_good = (int *)cb_data;
>> + *m_good = 0;
>> + return 1;
>> +}
>> +
>> +static char *bisect_voc(char *revision_type)
>> +{
>> + if (!strcmp(revision_type, "bad"))
>> + return "bad|new";
>> + if (!strcmp(revision_type, "good"))
>> + return "good|old";
>> +
>> + return NULL;
>> +}
>> +
>> +static int bisect_next_check(const struct bisect_terms *terms,
>> +  const char *current_term)
>> +{
>> + int missing_good = 1, missing_bad = 1;
>> + char *bad_ref = xstrfmt("refs/bisect/%s", terms->term_bad.buf);
>> + char *good_glob = xstrfmt("%s-*", terms->term_good.buf);
>> + char *bad_syn, *good_syn;
>> +
>> + if (ref_exists(bad_ref))
>> + missing_bad = 0;
>> + free(bad_ref);
>> +
>> + for_each_glob_ref_in(mark_good, good_glob, "refs/bisect/",
>> +  (void *) _good);
>> + free(good_glob);
>> +
>> + if (!missing_good && !missing_bad)
>> + return 0;
>> +
>> + if (!current_term)
>> + return -1;
>> +
>> + if (missing_good && !missing_bad && current_term &&
>> + !strcmp(current_term, terms->term_good.buf)) {
>> + char *yesno;
>> + /*
>> +  * have bad (or new) but not good (or old). We could bisect
>> +  * although this is less optimum.
>> +  */
>> + fprintf(stderr, _("Warning: bisecting only with a %s 
>> commit\n"),
>> + terms->term_bad.buf);
>> + if (!isatty(0))
>> + return 0;
>> + /*
>> +  * TRANSLATORS: Make sure to include [Y] and [n] in your
>> +  * translation. The program will only accept English input
>> +  * at this point.
>> +  */
>> + yesno = git_prompt(_("Are you sure [Y/n]? "), PROMPT_ECHO);
>> + if (starts_with(yesno, "N") || starts_with(yesno, "n"))
>> + return -1;
>> +
>> + return 0;
>> + }
>> + bad_syn = xstrdup(bisect_voc("bad"));
>> + good_syn = xstrdup(bisect_voc("good"));
>> + if (!is_empty_or_missing_file(git_path_bisect_start())) {
>> + error(_("You need to give me at least one %s and "
>> + "%s revision. You can use \"git bisect %s\" "
>> + "and \"git bisect %s\" for that. \n"),
>> + bad_syn, good_syn, bad_syn, good_syn);
>> + free(bad_syn);
>> + free(good_syn);
>> + return -1;
>> + }
>> + else {
>> + error(_("You need to start by \"git bisect start\". You "
>> + "then need to give me at least one %s and %s "
>> + "revision. You can use \"git bisect %s\" and "
>> + "\"git bisect %s\" for that.\n"),
>> + good_syn, bad_syn, bad_syn, good_syn);
>> + free(bad_syn);
>> + free(good_syn);
>> + return -1;
>> + }
>> + free(bad_syn);
>> + free(good_syn);
>> +
>> + return 0;
>
> This one looks OK, but I think the same "Wouldn't it become cleaner
> to have a 'finish:' label at the end and jump there?" comment
> applies to this implementation, too.

For this goto can simply things. Will do!

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

2016-08-27 Thread Pranit Bauva
Hey Junio,

On Thu, Aug 25, 2016 at 4:00 AM, Junio C Hamano  wrote:
> Pranit Bauva  writes:
>
>> +struct bisect_terms {
>> + struct strbuf term_good;
>> + struct strbuf term_bad;
>> +};
>
> I think "struct strbuf" is overrated.  For things like this, where
> these fields will never change once it is set (and setting it is
> done atomically, not incrementally), there is no good reason to use
> define the fields as strbuf.
>
> Only because you chose to use strbuf for these two fields, you have
> to make unnecessarily copies of argv[] in the command parser, and
> you have to remember to discard these copies later.
>
> I think you can just say "const char *" in this case.

Using struct strbuf is not really overrated but in fact required. But
yes, for this patch it might seem as overrated. In the shell code
initally TERM_GOOD is set to "good" while TERM_BAD is set to "bad".
Now there are a lot of instances (one of which is bisect_start()
function) where this can change. So if we keep it as "const char *",
it would be right to change the value of it after wards. And we cannot
keep it as "char []" because we don't know its size before hand.

>> +static int bisect_write(const char *state, const char *rev,
>> + const struct bisect_terms *terms, int nolog)
>> +{
>> + struct strbuf tag = STRBUF_INIT;
>> + struct strbuf commit_name = STRBUF_INIT;
>> + struct object_id oid;
>> + struct commit *commit;
>> + struct pretty_print_context pp = {0};
>> + FILE *fp;
>> +
>> + if (!strcmp(state, terms->term_bad.buf))
>> + strbuf_addf(, "refs/bisect/%s", state);
>> + else if (one_of(state, terms->term_good.buf, "skip", NULL))
>> + strbuf_addf(, "refs/bisect/%s-%s", state, rev);
>> + else
>> + return error(_("Bad bisect_write argument: %s"), state);
>
> OK.
>
>> + if (get_oid(rev, )) {
>> + strbuf_release();
>> + return error(_("couldn't get the oid of the rev '%s'"), rev);
>> + }
>> +
>> + if (update_ref(NULL, tag.buf, oid.hash, NULL, 0,
>> +UPDATE_REFS_MSG_ON_ERR)) {
>> + strbuf_release();
>> + return -1;
>> + }
>> + strbuf_release();
>> +
>> + fp = fopen(git_path_bisect_log(), "a");
>> + if (!fp)
>> + return error_errno(_("couldn't open the file '%s'"), 
>> git_path_bisect_log());
>> +
>> + commit = lookup_commit_reference(oid.hash);
>> + format_commit_message(commit, "%s", _name, );
>> + fprintf(fp, "# %s: [%s] %s\n", state, sha1_to_hex(oid.hash),
>> + commit_name.buf);
>> + strbuf_release(_name);
>> +
>> + if (!nolog)
>> + fprintf(fp, "git bisect %s %s\n", state, rev);
>> +
>> + fclose(fp);
>> + return 0;
>
> You seem to be _release()ing tag all over the place.
>
> Would it make sense to have a single clean-up label at the end of
> function, introduce a "int retval" variable and set it to -1 (or
> whatever) when an error is detected and "goto" to the label?  It may
> make it harder to make such a leak.  That is, to end the function
> more like:

I think I could use goto for this function.

> finish:
> if (fp)
> fclose(fp);
> strbuf_release();
> strbuf_release(_name);
> return retval;
> }
>
> and have sites with potential errors do something like this:
>
> if (update_ref(...)) {
> retval = -1;
> goto finish;
> }
>
>> + struct bisect_terms terms;
>> + bisect_terms_init();
>
> With the type of "struct bisect_terms" members corrected, you do not
> even need the _init() function.

Discussed above.

>> @@ -182,24 +251,38 @@ int cmd_bisect__helper(int argc, const char **argv, 
>> const char *prefix)
>>   usage_with_options(git_bisect_helper_usage, options);
>>
>>   switch (cmdmode) {
>> + int nolog;
>>   case NEXT_ALL:
>>   return bisect_next_all(prefix, no_checkout);
>>   case WRITE_TERMS:
>>   if (argc != 2)
>>   die(_("--write-terms requires two arguments"));
>> - return write_terms(argv[0], argv[1]);
>> + res = write_terms(argv[0], argv[1]);
>> + break;
>>   case BISECT_CLEAN_STATE:
>>   if (argc != 0)
>>   die(_("--bisect-clean-state requires no arguments"));
>> - return bisect_clean_state();
>> + res = bisect_clean_state();
>> + break;
>>   case BISECT_RESET:
>>   if (argc > 1)
>>   die(_("--bisect-reset requires either zero or one 
>> arguments"));
>> - return bisect_reset(argc ? argv[0] : NULL);
>> + res = bisect_reset(argc ? argv[0] : NULL);
>> + break;
>>   case CHECK_EXPECTED_REVS:
>> - return 

Re: [PATCH v14 08/27] bisect--helper: `is_expected_rev` & `check_expected_revs` shell function in C

2016-08-27 Thread Pranit Bauva
Hey Junio,

On Thu, Aug 25, 2016 at 3:43 AM, Junio C Hamano  wrote:
> Pranit Bauva  writes:
>
>> +static int is_expected_rev(const char *expected_hex)
>> +{
>> + struct strbuf actual_hex = STRBUF_INIT;
>> + int res = 0;
>> + if (strbuf_read_file(_hex, git_path_bisect_expected_rev(), 0) 
>> >= 0) {
>> + strbuf_trim(_hex);
>> + res = !strcmp(actual_hex.buf, expected_hex);
>
> If it is known to have 40-hex:
>
>  (1) accepting ">= 0" seems way too lenient.  You only expect a
>  41-byte file (or 42 if somebody would write CRLF, but I do not
>  think anybody other than yourself is expected to write into
>  this file, and you do not write CRLF yourself);
>
>  (2) strbuf_trim() is overly loose.  You only want to trim the
>  terimnating LF and it is an error to have other trailing
>  whitespaces.
>
> I think the latter is not a new problem and it is OK to leave it
> as-is; limiting (1) to >= 40 may still be a good change, though,
> because it makes the intention of the code clearer.

I can do the first change. Since this wasn't present in the shell
code, I will also mention it in the commit message.

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

2016-08-27 Thread Jakub Narębski
W dniu 27.08.2016 o 00:42, Junio C Hamano  pisze:
> Stefan Beller  writes:

> -- >8 --
> From: Beat Bolli 

???

--
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: stable as subset of develop

2016-08-27 Thread Jakub Narębski
W dniu 27.08.2016 o 04:29, Brett Porter pisze:
> 
> 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)

If you are really using topic branches, a better workflow would be
to make use of them.  That is, do the development of new features
on topic branches, test them in relative isolation, and when deemed
ready merge them into development branch, for more testing (including
integration testing).

Then, those topic branches that are considered stable are merged
into stable branch ("trunk").  You can think of it as picking
features to have in stable.

Take a look at Junio's blog posts about this topic.

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

That looks a bit like reimplementation of cherry-picking.

Also, I think you would loose the ability to run git-bisect to find
bad commits.
 
> This could help with applying successively more intense testing over
> time and chase down where problems arose.

Reiterationg: if you are using topic branches, use topic-branch workflow.

-- 
Jakub Narębski

author of "Mastering Git"
https://www.packtpub.com/application-development/mastering-git
--
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

2016-08-27 Thread Johannes Sixt

Am 26.08.2016 um 20:24 schrieb 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.


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.


While it may be easier to read due to the extra mark-up, the resulting 
text where such a quotation appears does not flow well, IMO. A commit 
message text that references another commit reads more fluently without 
the quotes around the summary line because the quoted text is not so 
much a quotation that must be marked, but a parenthetical statement.


I absolutely welcome the proposed change to gitk, because I always edit 
out the double-quotes.


-- Hannes

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