Git development
 help / color / mirror / Atom feed
* Re: [RFC PATCH] send-email: allow a custom hook to prevent sending email
From: Stefan Beller @ 2016-12-09 23:56 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Brandon Williams, Jeff King, git@vger.kernel.org
In-Reply-To: <xmqqshpwpsn8.fsf@gitster.mtv.corp.google.com>

On Fri, Dec 9, 2016 at 3:52 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Stefan Beller <sbeller@google.com> writes:
>
>> So you are suggesting to
>> * have the check later in the game (e.g. just after asking
>>    "Send this email? ([y]es|[n]o|[q]uit|[a]ll): " as then other information
>>   such as additional @to @cc are available.
>
> Yeah, probably before the loop starts asking that question for each
> message.  And hook does not necessarily need to cause the program to
> die.  The question can be reworded to "Your hook says no, but do you
> really want to send it?",

You could, but that would be inconsistent with the "*** SUBJECT ***"
treatment, which currently dies. That could also ask "do you really want
to send out an unfinished series" and continue if the user wants.

I assume we want to be consistent with the existing UI and just ask the
user to use force instead?

^ permalink raw reply

* Re: [RFC PATCH] send-email: allow a custom hook to prevent sending email
From: Junio C Hamano @ 2016-12-09 23:52 UTC (permalink / raw)
  To: Stefan Beller; +Cc: Brandon Williams, Jeff King, git@vger.kernel.org
In-Reply-To: <CAGZ79kaqw=XqrNF5+Ta8CwcD7FyA853UQUdMxHmBAaMHPMHrXg@mail.gmail.com>

Stefan Beller <sbeller@google.com> writes:

> So you are suggesting to
> * have the check later in the game (e.g. just after asking
>    "Send this email? ([y]es|[n]o|[q]uit|[a]ll): " as then other information
>   such as additional @to @cc are available.

Yeah, probably before the loop starts asking that question for each
message.  And hook does not necessarily need to cause the program to
die.  The question can be reworded to "Your hook says no, but do you
really want to send it?",

^ permalink raw reply

* Re: [PATCH v2 12/16] pathspec: create parse_long_magic function
From: Stefan Beller @ 2016-12-09 23:52 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Brandon Williams, git@vger.kernel.org, Duy Nguyen
In-Reply-To: <xmqqwpf8pt0g.fsf@gitster.mtv.corp.google.com>

On Fri, Dec 9, 2016 at 3:44 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Brandon Williams <bmwill@google.com> writes:
>
>> Factor out the logic responsible for parsing long magic into its own
>> function.  As well as hoist the prefix check logic outside of the inner
>> loop as there isn't anything that needs to be done after matching
>> "prefix:".
>>
>> Signed-off-by: Brandon Williams <bmwill@google.com>
>
> These refactoring changes look like they are all going in the good
> direction.  Stefan's :(attr:<attribute spec>)path" changes however
> have severe conflicts (e.g. the topic already does something similar
> to this step and calls the factored-out function eat_long_magic()).
>
> My gut feeling is that we probably should ask Stefan's series to be
> rebased on top of this series that cleans up pathspec implementation,
> once it stabilizes.

Very much so.

Jonathan Nieder mentioned off list that he prefers to see that
series rerolled without mutexes if possible. That is possible by
creating the questions "struct attr_check" before preloading the
index and then using the read only questions in the threaded code,
to obtain answers fast; also no need for a mutex.

I did not look into that yet, though. So I think you could discard that
series (again) until I find time to either redo the series or
resend it with a proper explanation on why the approach above
is not feasible.

>  We could probably go the other way around, but
> logically it makes more sense to build "pathspec can also match
> using attributes information" on top of a refactored codebase.
>
> Thoughts?

Please let the refactoring in in favor of the attr series.

^ permalink raw reply

* Re: [PATCH v2 12/16] pathspec: create parse_long_magic function
From: Junio C Hamano @ 2016-12-09 23:44 UTC (permalink / raw)
  To: Brandon Williams; +Cc: git, sbeller, pclouds
In-Reply-To: <1481223550-65277-13-git-send-email-bmwill@google.com>

Brandon Williams <bmwill@google.com> writes:

> Factor out the logic responsible for parsing long magic into its own
> function.  As well as hoist the prefix check logic outside of the inner
> loop as there isn't anything that needs to be done after matching
> "prefix:".
>
> Signed-off-by: Brandon Williams <bmwill@google.com>

These refactoring changes look like they are all going in the good
direction.  Stefan's :(attr:<attribute spec>)path" changes however
have severe conflicts (e.g. the topic already does something similar
to this step and calls the factored-out function eat_long_magic()).

My gut feeling is that we probably should ask Stefan's series to be
rebased on top of this series that cleans up pathspec implementation,
once it stabilizes.  We could probably go the other way around, but
logically it makes more sense to build "pathspec can also match
using attributes information" on top of a refactored codebase.

Thoughts?

^ permalink raw reply

* Re: [PATCH 0/4] doc: fixes to gitcore-tutorial.txt
From: Junio C Hamano @ 2016-12-09 23:13 UTC (permalink / raw)
  To: Kristoffer Haugsbakk; +Cc: git
In-Reply-To: <20161209155112.2112-1-kristoffer.haugsbakk@gmail.com>

Kristoffer Haugsbakk <kristoffer.haugsbakk@gmail.com> writes:

> This series of patches attempts to fix some minor mistakes in
> gitcore-tutorial.txt that I found while reading it.  They are all
> concerned with grammar and things like accidentally omitted words.

Grammar is not my forte, so even though I'll queue them as-is
because I didn't spot anything wrong in them and thought they are
all improvements, I'd appreciate somebody else to lend an eye or two
over them.

Thanks.

^ permalink raw reply

* Re: [PATCH] describe: add tests for unusual graphs
From: Junio C Hamano @ 2016-12-09 23:12 UTC (permalink / raw)
  To: Quinn Grier; +Cc: git
In-Reply-To: <20161209131141.3096-1-quinn@quinngrier.com>

Quinn Grier <quinn@quinngrier.com> writes:

> git describe may give incorrect results if there are backdated commits
> or multiple roots. This commit adds two test_expect_failure tests that
> demonstrate these problems.

I am not sure if this is a good patch to take.  test_expect_failure
is to demonstrate an incorrect behaviour that we wish to correct
later, but I do not think these demonstrate incorrect behaviours to
begin with.

For example, the latter one seems to expect that by asking to
describe D in this picture

> +#
> +# A---B*--D master
> +#        /
> +#       C* other
> +#

you seem to expect the description is based on B.  

It is not at all clear why it is incorrect if the description were
made based on C.  If D were described relative to A, ignoring B,
then I understand why that result is incorrect and I would agree
that describing D in terms of B is more correct.  But I do not think
that is what the test is trying to demonstrate.

But it is hard to guess only from looking at the test and the
proposed log message, because it does not say what makes you think
the behaviour you saw was incorrect.

^ permalink raw reply

* Re: [PATCHv6 4/7] worktree: get worktrees from submodules
From: Stefan Beller @ 2016-12-09 23:10 UTC (permalink / raw)
  To: Brandon Williams; +Cc: Duy Nguyen, Git Mailing List, Junio C Hamano
In-Reply-To: <20161209230029.GF88637@google.com>

On Fri, Dec 9, 2016 at 3:00 PM, Brandon Williams <bmwill@google.com> wrote:
> On 12/08, Stefan Beller wrote:
>> On Thu, Dec 8, 2016 at 2:09 AM, Duy Nguyen <pclouds@gmail.com> wrote:
>> > On Thu, Dec 8, 2016 at 8:46 AM, Stefan Beller <sbeller@google.com> wrote:
>> >>
>> >>         worktree = xcalloc(1, sizeof(*worktree));
>> >>         worktree->path = strbuf_detach(&worktree_path, NULL);
>> >> @@ -101,7 +101,8 @@ static struct worktree *get_main_worktree(void)
>> >
>> > All the good stuff is outside context lines again.. Somewhere between
>> > here we call add_head_info() which calls resolve_ref_unsafe(), which
>> > always uses data from current repo, not the submodule we want it to
>> > look at.
>>
>> Unrelated side question: What would you think of "variable context line
>> configuration" ? e.g. you could configure it to include anything from
>> up that line
>> that is currently shown after the @@ which is the function signature line.
>>
>> As to the add_head_info/resolve_ref_unsafe what impact does that have?
>> It produces a wrong head info but AFAICT it will never die(), such that for the
>> purposes of this series (which only wants to know if a submodule uses the
>> worktree feature) it should be fine.
>>
>> It is highly misleading though for others to build upon this.
>> So maybe I'll only add the functionality internally in worktree.c
>> and document why the values are wrong, and only expose the
>> "int submodule_uses_worktrees(const char *path)" ?
>>
>> >> @@ -209,6 +211,30 @@ struct worktree **get_worktrees(unsigned flags)
>> >>         return list;
>> >
>> > Right before this line is mark_current_worktree(), which in turn calls
>> > get_git_dir() and not suitable for peeking into another repository the
>> > way submodule code does. get_worktree_git_dir() called within that
>> > function shares the same problem.
>>
>> It actually works correctly: "No submodule is the current worktree".
>>
>>
>> >
>> >>  }
>> >>
>> >> +struct worktree **get_worktrees(unsigned flags)
>> >> +{
>> >> +       return get_worktrees_internal(get_git_common_dir(), flags);
>> >> +}
>> >> +
>> >> +struct worktree **get_submodule_worktrees(const char *path, unsigned flags)
>> >> +{
>> >> +       char *submodule_gitdir;
>> >> +       struct strbuf sb = STRBUF_INIT;
>> >> +       struct worktree **ret;
>> >> +
>> >> +       submodule_gitdir = git_pathdup_submodule(path, "%s", "");
>> >> +       if (!submodule_gitdir)
>> >> +               return NULL;
>> >> +
>> >> +       /* the env would be set for the superproject */
>> >> +       get_common_dir_noenv(&sb, submodule_gitdir);
>> >
>> > Technically we need to read submodule_gitdir/.config and see if we can
>> > understand core.repositoryformatversion, or find any unrecognized
>> > extensions. But the problem is not specific to this code. And fixing
>> > it is no small task. But perhaps we could call a dummy
>> > validate_submodule_gitdir() here? Then when we implement that function
>> > for real, we don't have to search the entire code base to see where to
>> > put it.
>> >
>> > Kinda off-topic though. Feel free to ignore the above comment.
>>
>> ok I'll add a TODO/emptyfunction for that.
>
> So is using resolve_gitdir not ok when trying to see if a submodule has
> a gitdir at a particular path?
>

Well it depends on the question that you are trying to answer:

Assume we introduce a new repository format in Git version 3.0.
By some means the submodule repository is converted to the new format,
but the superproject is not. (e.g. it is auto migrating repositories that have
a lot of large blobs/files)

Now for some reason you use the older git 2.X in the superproject.
(e.g. the code is on a shared network mount, or git3 has a bug, or ...)

Then the code above may tell that the submodule doesn't use worktrees
(as we cannot make any assumptions on the new crazy repository format),
but in fact it is, so in this case we technically we would need to:

1) read the config
2) check the repository format version (if larger than we know of,
   assume the worst or just die?)

As the function in this patch is used for safe guarding we may want to be
extra cautious, another function that is using resolve_gitdir may have other
assumptions on what is ok even for newer repository formats.

^ permalink raw reply

* Re: [PATCH 2/3] difftool: chdir as early as possible
From: Junio C Hamano @ 2016-12-09 23:02 UTC (permalink / raw)
  To: David Aguilar; +Cc: Git ML
In-Reply-To: <20161209085848.10929-2-davvid@gmail.com>

David Aguilar <davvid@gmail.com> writes:

> @@ -182,10 +188,6 @@ EOF
>  		}
>  	}
>  
> -	# Go to the root of the worktree so that the left index files
> -	# are properly setup -- the index is toplevel-relative.
> -	chdir($workdir);
> -
>  	# Setup temp directories
>  	my $tmpdir = tempdir('git-difftool.XXXXX', CLEANUP => 0, TMPDIR => 1);
>  	my $ldir = "$tmpdir/left";

What codebase are you basing your work on?  I do not see these
removed four lines in my tree, so it seems that the patch is fixing
up some other fix I do not yet have.

> @@ -235,10 +237,10 @@ EOF
>  			symlink("$workdir/$file", "$rdir/$file") or
>  			exit_cleanup($tmpdir, 1);
>  		} else {
> -			copy("$workdir/$file", "$rdir/$file") or
> +			copy($file, "$rdir/$file") or
>  			exit_cleanup($tmpdir, 1);
>  
> -			my $mode = stat("$workdir/$file")->mode;
> +			my $mode = stat($file)->mode;
>  			chmod($mode, "$rdir/$file") or
>  			exit_cleanup($tmpdir, 1);
>  		}
> @@ -430,10 +432,10 @@ sub dir_diff
>  			$error = 1;
>  		} elsif (exists $tmp_modified{$file}) {
>  			my $mode = stat("$b/$file")->mode;
> -			copy("$b/$file", "$workdir/$file") or
> +			copy("$b/$file", $file) or
>  			exit_cleanup($tmpdir, 1);
>  
> -			chmod($mode, "$workdir/$file") or
> +			chmod($mode, $file) or
>  			exit_cleanup($tmpdir, 1);
>  		}
>  	}

^ permalink raw reply

* Re: [PATCHv6 4/7] worktree: get worktrees from submodules
From: Brandon Williams @ 2016-12-09 23:00 UTC (permalink / raw)
  To: Stefan Beller; +Cc: Duy Nguyen, Git Mailing List, Junio C Hamano
In-Reply-To: <CAGZ79kYtEUvuTX09sJm3C0rG0-BrBz4bN0FCs6E5d2jHhtKN6w@mail.gmail.com>

On 12/08, Stefan Beller wrote:
> On Thu, Dec 8, 2016 at 2:09 AM, Duy Nguyen <pclouds@gmail.com> wrote:
> > On Thu, Dec 8, 2016 at 8:46 AM, Stefan Beller <sbeller@google.com> wrote:
> >>
> >>         worktree = xcalloc(1, sizeof(*worktree));
> >>         worktree->path = strbuf_detach(&worktree_path, NULL);
> >> @@ -101,7 +101,8 @@ static struct worktree *get_main_worktree(void)
> >
> > All the good stuff is outside context lines again.. Somewhere between
> > here we call add_head_info() which calls resolve_ref_unsafe(), which
> > always uses data from current repo, not the submodule we want it to
> > look at.
> 
> Unrelated side question: What would you think of "variable context line
> configuration" ? e.g. you could configure it to include anything from
> up that line
> that is currently shown after the @@ which is the function signature line.
> 
> As to the add_head_info/resolve_ref_unsafe what impact does that have?
> It produces a wrong head info but AFAICT it will never die(), such that for the
> purposes of this series (which only wants to know if a submodule uses the
> worktree feature) it should be fine.
> 
> It is highly misleading though for others to build upon this.
> So maybe I'll only add the functionality internally in worktree.c
> and document why the values are wrong, and only expose the
> "int submodule_uses_worktrees(const char *path)" ?
> 
> >> @@ -209,6 +211,30 @@ struct worktree **get_worktrees(unsigned flags)
> >>         return list;
> >
> > Right before this line is mark_current_worktree(), which in turn calls
> > get_git_dir() and not suitable for peeking into another repository the
> > way submodule code does. get_worktree_git_dir() called within that
> > function shares the same problem.
> 
> It actually works correctly: "No submodule is the current worktree".
> 
> 
> >
> >>  }
> >>
> >> +struct worktree **get_worktrees(unsigned flags)
> >> +{
> >> +       return get_worktrees_internal(get_git_common_dir(), flags);
> >> +}
> >> +
> >> +struct worktree **get_submodule_worktrees(const char *path, unsigned flags)
> >> +{
> >> +       char *submodule_gitdir;
> >> +       struct strbuf sb = STRBUF_INIT;
> >> +       struct worktree **ret;
> >> +
> >> +       submodule_gitdir = git_pathdup_submodule(path, "%s", "");
> >> +       if (!submodule_gitdir)
> >> +               return NULL;
> >> +
> >> +       /* the env would be set for the superproject */
> >> +       get_common_dir_noenv(&sb, submodule_gitdir);
> >
> > Technically we need to read submodule_gitdir/.config and see if we can
> > understand core.repositoryformatversion, or find any unrecognized
> > extensions. But the problem is not specific to this code. And fixing
> > it is no small task. But perhaps we could call a dummy
> > validate_submodule_gitdir() here? Then when we implement that function
> > for real, we don't have to search the entire code base to see where to
> > put it.
> >
> > Kinda off-topic though. Feel free to ignore the above comment.
> 
> ok I'll add a TODO/emptyfunction for that.

So is using resolve_gitdir not ok when trying to see if a submodule has
a gitdir at a particular path?

-- 
Brandon Williams

^ permalink raw reply

* Re: [RFC PATCH] send-email: allow a custom hook to prevent sending email
From: Stefan Beller @ 2016-12-09 22:53 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Brandon Williams, Jeff King, git@vger.kernel.org
In-Reply-To: <xmqqfulwraq2.fsf@gitster.mtv.corp.google.com>

On Fri, Dec 9, 2016 at 2:36 PM, Junio C Hamano <gitster@pobox.com> wrote:
>
> I doubt that this is the best place to call this hook, because the
> called hook does not have access to information that may help it
> make a better decision.

As the commit message may elude, I chose this place as it would be
sufficient for checking for ChangeIds, missing signoffs, or even
rudimentary check for coding style and commit message line length.

>
> For example, because the hook gets one patchfile at a time, it does
> not have the entire picture (e.g. "are you sure you want 01/05,
> 02/05, 04/05 and 05/05 without 03/05?").  For another example, the
> hook does not have access to the decision git-send-email makes on
> various "parameters", which are computed based on the contents of
> the patchfiles and command line arguments at this point in the code.
> (e.g. @to, @cc, etc. are computed much later, so you cannot say "do
> not send anythnng outside corp by mistake" with this mechanism).
>

So you are suggesting to
* have the check later in the game (e.g. just after asking
   "Send this email? ([y]es|[n]o|[q]uit|[a]ll): " as then other information
  such as additional @to @cc are available.
* the hook should not just be called one file at a time, but rather
  we would give all file names via e.g. stdin. With the current code
  structure this contradicts the first point.

I wonder if we want to have multiple hooks for these different things
of either looking at the big picture or looking at each in detail.

For me currently I am only interested in the small picture thing.

Stefan

^ permalink raw reply

* Re: [RFC PATCH] send-email: allow a custom hook to prevent sending email
From: Junio C Hamano @ 2016-12-09 22:36 UTC (permalink / raw)
  To: Stefan Beller; +Cc: bmwill, peff, git
In-Reply-To: <20161209203449.17940-1-sbeller@google.com>

Stefan Beller <sbeller@google.com> writes:

> This custom hook could be used to prevent sending out e.g. patches
> with change ids or other information that upstream doesn't like to see
> or is not supposed to see.
>
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
>
> My first perl contribution to Git. :)
>
> Marked as RFC to gauge general interest before writing tests and documentation.
>
> Thanks,
> Stefan
>
>  git-send-email.perl | 9 +++++++++
>  1 file changed, 9 insertions(+)
>
> diff --git a/git-send-email.perl b/git-send-email.perl
> index da81be40cb..d3ebf666c3 100755
> --- a/git-send-email.perl
> +++ b/git-send-email.perl
> @@ -815,6 +815,15 @@ if (!$force) {
>  				. "Pass --force if you really want to send.\n";
>  		}
>  	}
> +	my @hook = ( $ENV{GIT_DIR}.'hooks/send-email', $f )
> +	if( -x $hook[0] ) {
> +		unless( system( @hook ) == 0 )
> +		{
> +			die "Refusing to send because the patch\n\t$f\n"
> +				. "was refused by the send-email hook."
> +				. "Pass --force if you really want to send.\n";
> +		}
> +	}
>  }

I doubt that this is the best place to call this hook, because the
called hook does not have access to information that may help it
make a better decision.  

For example, because the hook gets one patchfile at a time, it does
not have the entire picture (e.g. "are you sure you want 01/05,
02/05, 04/05 and 05/05 without 03/05?").  For another example, the
hook does not have access to the decision git-send-email makes on
various "parameters", which are computed based on the contents of
the patchfiles and command line arguments at this point in the code.
(e.g. @to, @cc, etc. are computed much later, so you cannot say "do
not send anythnng outside corp by mistake" with this mechanism).





^ permalink raw reply

* Re: [PATCH v6 01/16] Git.pm: add subroutines for commenting lines
From: Junio C Hamano @ 2016-12-09 22:23 UTC (permalink / raw)
  To: Johannes Schindelin
  Cc: Vasco Almeida, git, Jiang Xin,
	Ævar Arnfjörð Bjarmason, Jean-Noël AVILA,
	Jakub Narębski, David Aguilar
In-Reply-To: <alpine.DEB.2.20.1612091832310.23160@virtualbox>

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> Hi Vasco,
>
> On Fri, 9 Dec 2016, Vasco Almeida wrote:
>
>> A Ter, 22-11-2016 às 09:42 -0800, Junio C Hamano escreveu:
>> > The incremental update below looks sensible. We'd also want to
>> > protect this codepath from a misconfigured two-or-more byte sequence
>> > in core.commentchar, I would suspect, to be consistent.
>> 
>> Are the below changes alright for what you propose? It just checks if
>> the length of core.commentchar's value is 1, otherwise use '#' as the
>> comment_line_char.
>> As a note, when I set core.commentchar with "git config
>> core.commentChar 'batata'", I get the following error message when I
>> issue "git add -i":
>> 
>> error: core.commentChar should only be one character
>> fatal: bad config variable 'core.commentchar' in file '.git/config' at line 6
>
> This is exactly the same issue I fixed for rebase -i recently.

Yes, but the patch we see here punts "core.commentChar is not a
single-byte single-letter--panic!" case differently.  I think you
did "just take the first one" in "rebase -i", which I think is more
in line with the rest of the system, and this addition to Git.pm
should do the same, I think.

^ permalink raw reply

* Re: [REGRESSION 2.10.2] problematic "empty auth" changes
From: brian m. carlson @ 2016-12-09 22:19 UTC (permalink / raw)
  To: David Turner; +Cc: Johannes Schindelin, git
In-Reply-To: <1481231552.20894.20.camel@frank>

[-- Attachment #1: Type: text/plain, Size: 1040 bytes --]

On Thu, Dec 08, 2016 at 04:12:32PM -0500, David Turner wrote:
> I know of no reason that shouldn't work.  Indeed, it's what we use do
> internally.  So far, nobody has reported problems.  That said, we have
> exactly three sets of git servers that most users talk to (two different
> internal; and occasionally github.com for external stuff).  So our
> coverage is not very broad.
> 
> If you're going to do it, tho, don't just do it for Windows users -- do
> it for everyone.  Plenty of Unix clients connect to Windows-based auth
> systems.

Let me echo this.  This would make Kerberos (and probably other forms of
SPNEGO) work out of the box, which would reduce a lot of confusion that
people have.

I can confirm enabling http.emptyAuth works properly with Kerberos,
including with fallback to Basic, so I see no reason why we shouldn't do
it.
-- 
brian m. carlson / brian with sandals: Houston, Texas, US
+1 832 623 2791 | https://www.crustytoothpaste.net/~bmc | My opinion only
OpenPGP: https://keybase.io/bk2204

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 868 bytes --]

^ permalink raw reply

* Re: [BUG] Colon in remote urls
From: Johannes Sixt @ 2016-12-09 21:32 UTC (permalink / raw)
  To: Jeff King; +Cc: Klaus Ethgen, git
In-Reply-To: <20161209152219.ehfk475vdg4levop@sigill.intra.peff.net>

Am 09.12.2016 um 16:22 schrieb Jeff King:
> +const char *parse_alt_odb_entry(const char *string, int sep,
> +				struct strbuf *out)
> +{
> +	const char *p;
> +	int literal = 0;
> +
> +	strbuf_reset(out);
> +
> +	for (p = string; *p; p++) {
> +		if (literal) {
> +			strbuf_addch(out, *p);
> +			literal = 0;
> +		} else {
> +			if (*p == '\\')
> +				literal = 1;

There are too many systems out there that use a backslash in path names. 
I don't think it is wise to use it also as the quoting character.

> +			else if (*p == sep)
> +				return p + 1;
> +			else
> +				strbuf_addch(out, *p);
> +		}
> +	}
> +	return p;
> +}

-- Hannes


^ permalink raw reply

* [RFC PATCH] send-email: allow a custom hook to prevent sending email
From: Stefan Beller @ 2016-12-09 20:34 UTC (permalink / raw)
  To: bmwill, peff; +Cc: git, Stefan Beller

This custom hook could be used to prevent sending out e.g. patches
with change ids or other information that upstream doesn't like to see
or is not supposed to see.

Signed-off-by: Stefan Beller <sbeller@google.com>
---

My first perl contribution to Git. :)

Marked as RFC to gauge general interest before writing tests and documentation.

Thanks,
Stefan

 git-send-email.perl | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/git-send-email.perl b/git-send-email.perl
index da81be40cb..d3ebf666c3 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -815,6 +815,15 @@ if (!$force) {
 				. "Pass --force if you really want to send.\n";
 		}
 	}
+	my @hook = ( $ENV{GIT_DIR}.'hooks/send-email', $f )
+	if( -x $hook[0] ) {
+		unless( system( @hook ) == 0 )
+		{
+			die "Refusing to send because the patch\n\t$f\n"
+				. "was refused by the send-email hook."
+				. "Pass --force if you really want to send.\n";
+		}
+	}
 }
 
 if (defined $sender) {
-- 
2.11.0.rc2.49.ge1f3b0c.dirty


^ permalink raw reply related

* Re: [PATCH 14/16] pathspec: create strip submodule slash helpers
From: Brandon Williams @ 2016-12-09 20:30 UTC (permalink / raw)
  To: Stefan Beller; +Cc: git@vger.kernel.org, Duy Nguyen, Junio C Hamano
In-Reply-To: <CAGZ79kYVumubF58fdwejE2hvCLfgdVqTxn_w=S-CBFY-NdgqQg@mail.gmail.com>

On 12/09, Stefan Beller wrote:
> On Fri, Dec 9, 2016 at 11:18 AM, Brandon Williams <bmwill@google.com> wrote:
> > Factor out the logic responsible for stripping the trailing slash on
> > pathspecs referencing submodules into its own function.
> >
> > Change-Id: Icad62647c04b4195309def0e3db416203d14f9e4
> 
> I think we should come up with a solution to wipe out change ids
> before sending emails. ;)

Darn! Yeah maybe a hook or something :D

-- 
Brandon Williams

^ permalink raw reply

* Re: [PATCH v2 1/4] real_path: resolve symlinks by hand
From: Brandon Williams @ 2016-12-09 20:04 UTC (permalink / raw)
  To: Johannes Sixt
  Cc: git, sbeller, peff, jacob.keller, gitster, ramsay, tboegi,
	pclouds
In-Reply-To: <944a3e91-934a-7923-2b2a-639af81e707d@kdbg.org>

On 12/09, Johannes Sixt wrote:
> Am 09.12.2016 um 00:58 schrieb Brandon Williams:
> >The current implementation of real_path uses chdir() in order to resolve
> >symlinks.  Unfortunately this isn't thread-safe as chdir() affects a
> >process as a whole and not just an individual thread.  Instead perform
> >the symlink resolution by hand so that the calls to chdir() can be
> >removed, making real_path one step closer to being reentrant.
> >
> >Signed-off-by: Brandon Williams <bmwill@google.com>
> >---
> > abspath.c | 183 +++++++++++++++++++++++++++++++++++++++++---------------------
> > 1 file changed, 122 insertions(+), 61 deletions(-)
> >
> >diff --git a/abspath.c b/abspath.c
> >index 2825de8..92f2a29 100644
> >--- a/abspath.c
> >+++ b/abspath.c
> >@@ -11,8 +11,38 @@ int is_directory(const char *path)
> > 	return (!stat(path, &st) && S_ISDIR(st.st_mode));
> > }
> >
> >+/* removes the last path component from 'path' except if 'path' is root */
> >+static void strip_last_component(struct strbuf *path)
> >+{
> >+	if (path->len > offset_1st_component(path->buf)) {
> >+		char *last_slash = find_last_dir_sep(path->buf);
> >+		strbuf_setlen(path, last_slash - path->buf);
> >+	}
> >+}
> 
> This implementation is not correct because when the input is "/foo",
> the result is "" when it should be "/". Also, can the input be a
> non-normalized path? When the input is "foo///bar", should the
> result be "foo" or would "foo//" be an acceptable result? I think it
> should be the former. find_last_dir_sep() returns the last of the
> three slashes, not the first one. Therefore, I've rewritten the
> function thusly:
> 
> static void strip_last_component(struct strbuf *path)
> {
> 	size_t offset = offset_1st_component(path->buf);
> 	size_t len = path->len;
> 	while (offset < len && !is_dir_sep(path->buf[len - 1]))
> 		len--;
> 	while (offset < len && is_dir_sep(path->buf[len - 1]))
> 		len--;
> 	strbuf_setlen(path, len);
> }
> 

Thanks for that catch.  So your rewrite takes the offset of the 1st
component and ensures that we don't cut that part off.  It first strips
all non directory separators and then all directory separators.  This
way "/foo////bar" becomes "/foo" and as you pointed out "/foo" would
become "/".  The offset would also take care of windows drive letters
and the like.  Looks good.  Thanks!

> >+		strbuf_addbuf(&resolved, &next);
> >+
> >+		if (lstat(resolved.buf, &st)) {
> >+			/* error out unless this was the last component */
> >+			if (!(errno == ENOENT && !remaining.len)) {
> 
> Perhaps it was easier to _write_ the condition in this way, but I
> would have an easier time to _read_ it when it is
> 
> 			if (errno != ENOENT || remaining.len) {
> 

Yes I did write it out weird, mostly because it made the most sense for
what I was trying to accomplish (add path components must exist, except
for the very last one).  I'm fine applying DeMorgan's since it looks a
little cleaner.

> >+
> >+			if (is_absolute_path(symlink.buf)) {
> >+				/* absolute symlink; set resolved to root */
> >+				int offset = offset_1st_component(symlink.buf);
> >+				strbuf_reset(&resolved);
> >+				strbuf_add(&resolved, symlink.buf, offset);
> >+				strbuf_remove(&symlink, 0, offset);
> 
> Good. I would have expected some kind of "goto repeat;" because when
> we encounter a symlink to an absolute path, most, if not all, work
> done so far is obsoleted. But I haven't thought too deeply about
> this.

It made the most sense to just reuse the same looping condition that
I already had in place.  Resetting the resolved string to be the root
component of the absolute symlink made it easy to "throw away" all the
old work to allow us to start from scratch again.

-- 
Brandon Williams

^ permalink raw reply

* Re: [PATCH v2 0/4] road to reentrant real_path
From: Brandon Williams @ 2016-12-09 19:42 UTC (permalink / raw)
  To: Duy Nguyen
  Cc: Git Mailing List, Stefan Beller, Jeff King, Jacob Keller,
	Junio C Hamano, Ramsay Jones, Torsten Bögershausen,
	Johannes Sixt
In-Reply-To: <CACsJy8A2M_G34MeHh6vGsrf5ePOOduM6u=n17_EZLtu31uDAYg@mail.gmail.com>

On 12/09, Duy Nguyen wrote:
> On Fri, Dec 9, 2016 at 6:58 AM, Brandon Williams <bmwill@google.com> wrote:
> > diff --git a/setup.c b/setup.c
> > index fe572b8..0d9fdd0 100644
> > --- a/setup.c
> > +++ b/setup.c
> > @@ -254,10 +254,12 @@ int get_common_dir_noenv(struct strbuf *sb, const char *gitdir)
> >                 if (!is_absolute_path(data.buf))
> >                         strbuf_addf(&path, "%s/", gitdir);
> >                 strbuf_addbuf(&path, &data);
> > -               strbuf_addstr(sb, real_path(path.buf));
> > +               strbuf_realpath(sb, path.buf, 1);
> 
> This is not the same because of this hunk in strbuf_realpath()

Then perhaps I shouldn't make this change (and just leave it as is)
since the way real_path_internal/strbuf_realpath is written requires
that the strbuf being used for the resolved path only contains the
resolved path (see the lstat(resolved->buf &st) call).  Sidenote it
looks like strbuf_getcwd() also does a reset, though more subtlety,
since it just passes its buffer to getcwd().

> 
> > @@ -81,17 +73,18 @@ static const char *real_path_internal(const char *path, int die_on_error)
> >                         goto error_out;
> >         }
> >
> > -       strbuf_reset(&resolved);
> > +       strbuf_reset(resolved);
> >
> >         if (is_absolute_path(path)) {
> 
> But if you you remove that, then all (old) callers of
> strbuf_realpath() must do a strbuf_reset() in advance if needed
> (probably just real_path does) which sounds reasonable to me. You're
> probably want to be careful about the strbuf_reset() at the end of the
> function too.
> 
> Other than that, I think this diff looks nice.
> -- 
> Duy

-- 
Brandon Williams

^ permalink raw reply

* Re: [PATCH 14/16] pathspec: create strip submodule slash helpers
From: Stefan Beller @ 2016-12-09 19:40 UTC (permalink / raw)
  To: Brandon Williams; +Cc: git@vger.kernel.org, Duy Nguyen, Junio C Hamano
In-Reply-To: <1481311118-174146-1-git-send-email-bmwill@google.com>

On Fri, Dec 9, 2016 at 11:18 AM, Brandon Williams <bmwill@google.com> wrote:
> Factor out the logic responsible for stripping the trailing slash on
> pathspecs referencing submodules into its own function.
>
> Change-Id: Icad62647c04b4195309def0e3db416203d14f9e4

I think we should come up with a solution to wipe out change ids
before sending emails. ;)

> Signed-off-by: Brandon Williams <bmwill@google.com>
> ---
>  pathspec.c | 68 ++++++++++++++++++++++++++++++++++++++------------------------
>  1 file changed, 42 insertions(+), 26 deletions(-)
>
> diff --git a/pathspec.c b/pathspec.c
> index 84a57cf..4d9a6a0 100644
> --- a/pathspec.c
> +++ b/pathspec.c
> @@ -258,6 +258,44 @@ static const char *parse_element_magic(unsigned *magic, int *prefix_len,
>                 return parse_short_magic(magic, elem);
>  }
>
> +static void strip_submodule_slash_cheap(struct pathspec_item *item)
> +{
> +       if (item->len >= 1 && item->match[item->len - 1] == '/') {
> +               int i = cache_name_pos(item->match, item->len - 1);
> +
> +               if (i >= 0 && S_ISGITLINK(active_cache[i]->ce_mode)) {
> +                       item->len--;
> +                       item->match[item->len] = '\0';
> +               }
> +       }
> +}
> +
> +static void strip_submodule_slash_expensive(struct pathspec_item *item)
> +{
> +       int i;
> +
> +       for (i = 0; i < active_nr; i++) {
> +               struct cache_entry *ce = active_cache[i];
> +               int ce_len = ce_namelen(ce);
> +
> +               if (!S_ISGITLINK(ce->ce_mode))
> +                       continue;
> +
> +               if (item->len <= ce_len || item->match[ce_len] != '/' ||
> +                   memcmp(ce->name, item->match, ce_len))
> +                       continue;
> +
> +               if (item->len == ce_len + 1) {
> +                       /* strip trailing slash */
> +                       item->len--;
> +                       item->match[item->len] = '\0';
> +               } else {
> +                       die(_("Pathspec '%s' is in submodule '%.*s'"),
> +                           item->original, ce_len, ce->name);
> +               }
> +       }
> +}
> +
>  /*
>   * Take an element of a pathspec and check for magic signatures.
>   * Append the result to the prefix. Return the magic bitmap.
> @@ -278,7 +316,7 @@ static unsigned prefix_pathspec(struct pathspec_item *item, unsigned flags,
>         unsigned magic = 0, element_magic = 0;
>         const char *copyfrom = elt;
>         char *match;
> -       int i, pathspec_prefix = -1;
> +       int pathspec_prefix = -1;
>
>         /* PATHSPEC_LITERAL_PATH ignores magic */
>         if (flags & PATHSPEC_LITERAL_PATH) {
> @@ -329,33 +367,11 @@ static unsigned prefix_pathspec(struct pathspec_item *item, unsigned flags,
>         item->len = strlen(item->match);
>         item->prefix = prefixlen;
>
> -       if ((flags & PATHSPEC_STRIP_SUBMODULE_SLASH_CHEAP) &&
> -           (item->len >= 1 && item->match[item->len - 1] == '/') &&
> -           (i = cache_name_pos(item->match, item->len - 1)) >= 0 &&
> -           S_ISGITLINK(active_cache[i]->ce_mode)) {
> -               item->len--;
> -               match[item->len] = '\0';
> -       }
> +       if (flags & PATHSPEC_STRIP_SUBMODULE_SLASH_CHEAP)
> +               strip_submodule_slash_cheap(item);
>
>         if (flags & PATHSPEC_STRIP_SUBMODULE_SLASH_EXPENSIVE)
> -               for (i = 0; i < active_nr; i++) {
> -                       struct cache_entry *ce = active_cache[i];
> -                       int ce_len = ce_namelen(ce);
> -
> -                       if (!S_ISGITLINK(ce->ce_mode))
> -                               continue;
> -
> -                       if (item->len <= ce_len || match[ce_len] != '/' ||
> -                           memcmp(ce->name, match, ce_len))
> -                               continue;
> -                       if (item->len == ce_len + 1) {
> -                               /* strip trailing slash */
> -                               item->len--;
> -                               match[item->len] = '\0';
> -                       } else
> -                               die (_("Pathspec '%s' is in submodule '%.*s'"),
> -                                    elt, ce_len, ce->name);
> -               }
> +               strip_submodule_slash_expensive(item);
>
>         if (magic & PATHSPEC_LITERAL)
>                 item->nowildcard_len = item->len;
> --
> 2.8.0.rc3.226.g39d4020
>

^ permalink raw reply

* Re: [REGRESSION 2.10.2] problematic "empty auth" changes
From: Junio C Hamano @ 2016-12-09 19:31 UTC (permalink / raw)
  To: Johannes Schindelin; +Cc: David Turner, git
In-Reply-To: <alpine.DEB.2.20.1612091048540.23160@virtualbox>

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> It would be different, of course, if http.emptyAuth would *not* allow the
> user to type their credentials when accessing something like
> https://github.com/dscho/shhh-secret-repository, *only* trying the login
> credentials. But that is not the case, with http.emptyAuth=true, login
> credentials are attempted first, and when they fail, the user is still
> asked interactively for their credentials.
>
> All I can see is that this would be *an improvement*: corporate users
> trying to access a Git repository that requires their login credentials
> would now not even need to enter empty user name/password.

Yup, my thought process after seeing your first message to David
exactly mirrored the above two paragraphs.  It sounds like you two
have a good plan ;-)

Thanks.

> This alone would be already a good reason to change the default, IMHO.
>
> So here is my plan:
>
> - change the default of http.emptyAuth to true in the next Git for Windows
>   version
>
> - publish a prerelease for early adopters to test
>
> - contribute this patch here on the Git mailing list, in the hope that it
>   will make it into the next major version
>
> Ciao,
> Dscho

^ permalink raw reply

* Re: BUG: "cherry-pick A..B || git reset --hard OTHER"
From: Stephan Beyer @ 2016-12-09 19:28 UTC (permalink / raw)
  To: Junio C Hamano, Duy Nguyen
  Cc: Git Mailing List, Christian Couder, SZEDER Gábor
In-Reply-To: <e0780f7c-ccb4-29fe-3d72-80e45a202f65@gmx.net>

On 12/09/2016 08:24 PM, Stephan Beyer wrote:
> t3510 also shows another use-case for --quit: the title says it all:
> "cherry-pick --quit" to "cherry-pick --abort"

I should've read what I actually pasted.
I wanted to paste: '--quit keeps HEAD and conflicted index intact'

Sorry for making no sense ;)

> With this additional information, I'd vote to keep --quit/--forget and
> just make it consistent.

Now!

~Stephan

^ permalink raw reply

* Re: BUG: "cherry-pick A..B || git reset --hard OTHER"
From: Stephan Beyer @ 2016-12-09 19:24 UTC (permalink / raw)
  To: Junio C Hamano, Duy Nguyen
  Cc: Git Mailing List, Christian Couder, SZEDER Gábor
In-Reply-To: <xmqq8trprn7f.fsf@gitster.mtv.corp.google.com>

Hi Junio,

On 12/09/2016 07:07 PM, Junio C Hamano wrote:
> Duy Nguyen <pclouds@gmail.com> writes:
>> Having the same operation with different names only increases git
>> reputation of bad/inconsistent UI. Either forget is renamed to quit,
>> or vice versa. I prefer forget, but the decision is yours and the
>> community's. So I'm sending two patches to rename in either direction.
>> You can pick one.
> 
> I actually was advocating to remove both by making --abort saner.
> With an updated --abort that behaves saner, is "rebase --forget"
> still necessary?

A quick change in t3407 of the "rebase --forget" test to use "rebase
--abort" failed.  That's because it checks the use-case of
forgetting/aborting without changing the HEAD.  So --abort makes a
rollback, --forget just keeps the current head.  I am not sure if that
tested use-case is a real use-case though.

A quick change in the pristine_detach function in t3510 and t3511 from
"cherry-pick --quit" to "cherry-pick --abort" works when one ignores the
return value of "cherry-pick --abort". The "--quit" is used here to
ensure a clean cherry-pick state, and --quit always succeeds, even if no
cherry-pick is in progress.  That may be a real use-case somehow that
could also be used for "rebase --forget"

t3510 also shows another use-case for --quit: the title says it all:
"cherry-pick --quit" to "cherry-pick --abort"

With this additional information, I'd vote to keep --quit/--forget and
just make it consistent.

~Stephan


^ permalink raw reply

* Re: [PATCH 02/17] dir: convert create_simplify to use the pathspec struct interface
From: Brandon Williams @ 2016-12-09 19:23 UTC (permalink / raw)
  To: Duy Nguyen; +Cc: Git Mailing List, Stefan Beller, Junio C Hamano
In-Reply-To: <CACsJy8Cc6hE1Rbqjrc93xDMc0UUm0yMh0A-fyu3dfJ2G1jhENQ@mail.gmail.com>

On 12/09, Duy Nguyen wrote:
> On Fri, Dec 9, 2016 at 1:19 AM, Brandon Williams <bmwill@google.com> wrote:
> > On 12/08, Duy Nguyen wrote:
> >> On Thu, Dec 8, 2016 at 7:03 AM, Brandon Williams <bmwill@google.com> wrote:
> >> > On 12/07, Duy Nguyen wrote:
> >> >> On Wed, Dec 7, 2016 at 4:51 AM, Brandon Williams <bmwill@google.com> wrote:
> >> >> > Convert 'create_simplify()' to use the pathspec struct interface from
> >> >> > using the '_raw' entry in the pathspec.
> >> >>
> >> >> It would be even better to kill this create_simplify() and let
> >> >> simplify_away() handle struct pathspec directly.
> >> >>
> >> >> There is a bug in this code, that might have been found if we
> >> >> simpify_away() handled pathspec directly: the memcmp() in
> >> >> simplify_away() will not play well with :(icase) magic. My bad. If
> >> >> :(icase) is used, the easiest/safe way is simplify nothing. Later on
> >> >> maybe we can teach simplify_away() to do strncasecmp instead. We could
> >> >> ignore exclude patterns there too (although not excluding is not a
> >> >> bug).
> >> >
> >> > So are you implying that the simplify struct needs to be killed?  That
> >> > way the pathspec struct itself is being passed around instead?
> >>
> >> Yes. simplify struct was a thing when pathspec was an array of char *.
> >> At this point I think it can retire (when we have time to retire it)
> >
> > Alright, then for now I can leave this change as is and have a follow up
> > series that kills the simplify struct.
> 
> Do let me know if you decide to drop it, so I can put it back in my backlog.

K will do

-- 
Brandon Williams

^ permalink raw reply

* [PATCH 14/16] pathspec: create strip submodule slash helpers
From: Brandon Williams @ 2016-12-09 19:18 UTC (permalink / raw)
  To: git; +Cc: Brandon Williams, sbeller, pclouds, gitster
In-Reply-To: <CACsJy8AX6C8Tux9_8ZynBMNS2EW2pKQOGK8k0hVmbWvbZ8pa=Q@mail.gmail.com>

Factor out the logic responsible for stripping the trailing slash on
pathspecs referencing submodules into its own function.

Change-Id: Icad62647c04b4195309def0e3db416203d14f9e4
Signed-off-by: Brandon Williams <bmwill@google.com>
---
 pathspec.c | 68 ++++++++++++++++++++++++++++++++++++++------------------------
 1 file changed, 42 insertions(+), 26 deletions(-)

diff --git a/pathspec.c b/pathspec.c
index 84a57cf..4d9a6a0 100644
--- a/pathspec.c
+++ b/pathspec.c
@@ -258,6 +258,44 @@ static const char *parse_element_magic(unsigned *magic, int *prefix_len,
 		return parse_short_magic(magic, elem);
 }
 
+static void strip_submodule_slash_cheap(struct pathspec_item *item)
+{
+	if (item->len >= 1 && item->match[item->len - 1] == '/') {
+		int i = cache_name_pos(item->match, item->len - 1);
+
+		if (i >= 0 && S_ISGITLINK(active_cache[i]->ce_mode)) {
+			item->len--;
+			item->match[item->len] = '\0';
+		}
+	}
+}
+
+static void strip_submodule_slash_expensive(struct pathspec_item *item)
+{
+	int i;
+
+	for (i = 0; i < active_nr; i++) {
+		struct cache_entry *ce = active_cache[i];
+		int ce_len = ce_namelen(ce);
+
+		if (!S_ISGITLINK(ce->ce_mode))
+			continue;
+
+		if (item->len <= ce_len || item->match[ce_len] != '/' ||
+		    memcmp(ce->name, item->match, ce_len))
+			continue;
+
+		if (item->len == ce_len + 1) {
+			/* strip trailing slash */
+			item->len--;
+			item->match[item->len] = '\0';
+		} else {
+			die(_("Pathspec '%s' is in submodule '%.*s'"),
+			    item->original, ce_len, ce->name);
+		}
+	}
+}
+
 /*
  * Take an element of a pathspec and check for magic signatures.
  * Append the result to the prefix. Return the magic bitmap.
@@ -278,7 +316,7 @@ static unsigned prefix_pathspec(struct pathspec_item *item, unsigned flags,
 	unsigned magic = 0, element_magic = 0;
 	const char *copyfrom = elt;
 	char *match;
-	int i, pathspec_prefix = -1;
+	int pathspec_prefix = -1;
 
 	/* PATHSPEC_LITERAL_PATH ignores magic */
 	if (flags & PATHSPEC_LITERAL_PATH) {
@@ -329,33 +367,11 @@ static unsigned prefix_pathspec(struct pathspec_item *item, unsigned flags,
 	item->len = strlen(item->match);
 	item->prefix = prefixlen;
 
-	if ((flags & PATHSPEC_STRIP_SUBMODULE_SLASH_CHEAP) &&
-	    (item->len >= 1 && item->match[item->len - 1] == '/') &&
-	    (i = cache_name_pos(item->match, item->len - 1)) >= 0 &&
-	    S_ISGITLINK(active_cache[i]->ce_mode)) {
-		item->len--;
-		match[item->len] = '\0';
-	}
+	if (flags & PATHSPEC_STRIP_SUBMODULE_SLASH_CHEAP)
+		strip_submodule_slash_cheap(item);
 
 	if (flags & PATHSPEC_STRIP_SUBMODULE_SLASH_EXPENSIVE)
-		for (i = 0; i < active_nr; i++) {
-			struct cache_entry *ce = active_cache[i];
-			int ce_len = ce_namelen(ce);
-
-			if (!S_ISGITLINK(ce->ce_mode))
-				continue;
-
-			if (item->len <= ce_len || match[ce_len] != '/' ||
-			    memcmp(ce->name, match, ce_len))
-				continue;
-			if (item->len == ce_len + 1) {
-				/* strip trailing slash */
-				item->len--;
-				match[item->len] = '\0';
-			} else
-				die (_("Pathspec '%s' is in submodule '%.*s'"),
-				     elt, ce_len, ce->name);
-		}
+		strip_submodule_slash_expensive(item);
 
 	if (magic & PATHSPEC_LITERAL)
 		item->nowildcard_len = item->len;
-- 
2.8.0.rc3.226.g39d4020


^ permalink raw reply related

* Re: Any interest in 'git merge --continue' as a command
From: Junio C Hamano @ 2016-12-09 19:16 UTC (permalink / raw)
  To: Jeff King; +Cc: Chris Packham, GIT
In-Reply-To: <20161209091127.sxxczhfslrqsqs3m@sigill.intra.peff.net>

Jeff King <peff@peff.net> writes:

>> They knew about git rebase --continue (and git am and git cherry-pick)
>> but they were unsure how to "continue" a merge (it didn't help that
>> the advice saying to use 'git commit' was scrolling off the top of the
>> terminal). I know that using 'git commit' has been the standard way to
>> complete a merge but given other commands have a --continue should
>> merge have it as well?
>
> It seems like that would be in line with 35d2fffdb (Provide 'git merge
> --abort' as a synonym to 'git reset --merge', 2010-11-09), whose stated
> goal was providing consistency with other multi-command operations.
>
> I assume it would _just_ run a vanilla "git commit", and not try to do
> any trickery with updating the index (which could be disastrous).

If we were to have "merge --continue", I agree that it would be the
logical implementation.

There is nothing to "continue" in a stopped merge where Git asked
for help from the user, and because of that, I view the final "git
commit" as "concluding the merge", not "continuing".  "continue"
makes quite a lot of sense with rebase and cherry-pick A..B that
stopped; it concludes the current step and let it continue to
process the remainder.  So from that point of view, it somewhat
feels strange to call it "merge --continue", but it probably is just
me.




^ permalink raw reply


This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox