Re: [PATCH] am: add --check option

Re: [PATCH] am: add --check option

[Johannes Sixt]

Am 04.06.19 um 00:00 schrieb Drew DeVault:
> On Mon Jun 3, 2019 at 11:09 PM Johannes Sixt wrote:
>> I have to wonder how --check works when 'am' applies multiple patches.
>>
>> When the second patch in a patch series depends on that the first patch
>> is fully applied, what does --check do? Without the first patch applied,
>> then a naive check of the second patch will certainly fail, doesn't it?
> 
> Yeah, this was being discussed in another thread. It'll fail if the
> second patch relies on changes from the first. Open to suggestions on
> how to improve that, but I think it can be improved in a later patch.
> One solution would be to apply all of the patches and then roll back the
> head, but that would dirty the reflog and wouldn't work on a read-only
> filesystem (which it ought to, imo). We can't just say bugger this for a
> lark and ask people to use git-apply, because git-apply chokes on the
> typical email which isn't in the one specific format git-apply wants to
> see (git-am massages emails into that format before sending them to
> git-apply).
> 

You can 'git apply --cached' the patches on a temporary index. This
works as long as no merge is necessary, because that would require a
worktree.

-- Hannes

Re: [PATCH] am: add --check option

[Johannes Schindelin]

Hi,

On Tue, 4 Jun 2019, Johannes Sixt wrote:

> Am 04.06.19 um 00:00 schrieb Drew DeVault:
> > On Mon Jun 3, 2019 at 11:09 PM Johannes Sixt wrote:
> >> I have to wonder how --check works when 'am' applies multiple patches.
> >>
> >> When the second patch in a patch series depends on that the first patch
> >> is fully applied, what does --check do? Without the first patch applied,
> >> then a naive check of the second patch will certainly fail, doesn't it?
> >
> > Yeah, this was being discussed in another thread. It'll fail if the
> > second patch relies on changes from the first. Open to suggestions on
> > how to improve that, but I think it can be improved in a later patch.
> > One solution would be to apply all of the patches and then roll back the
> > head, but that would dirty the reflog and wouldn't work on a read-only
> > filesystem (which it ought to, imo). We can't just say bugger this for a
> > lark and ask people to use git-apply, because git-apply chokes on the
> > typical email which isn't in the one specific format git-apply wants to
> > see (git-am massages emails into that format before sending them to
> > git-apply).
> >
>
> You can 'git apply --cached' the patches on a temporary index. This
> works as long as no merge is necessary, because that would require a
> worktree.

For extra brownie points, this could be done in-memory, without writing
out any files.

Of course, while applying diffs should never need any merge, with the `-3`
option, merges might be necessary, still...

Ciao,
Johannes

Re: [PATCH] am: add --check option

[Christian Couder]

On Tue, Jun 4, 2019 at 8:03 AM Johannes Sixt <j6t@kdbg.org> wrote:
>
> Am 04.06.19 um 00:00 schrieb Drew DeVault:
> > On Mon Jun 3, 2019 at 11:09 PM Johannes Sixt wrote:
> >> I have to wonder how --check works when 'am' applies multiple patches.
> >>
> >> When the second patch in a patch series depends on that the first patch
> >> is fully applied, what does --check do? Without the first patch applied,
> >> then a naive check of the second patch will certainly fail, doesn't it?
> >
> > Yeah, this was being discussed in another thread. It'll fail if the
> > second patch relies on changes from the first. Open to suggestions on
> > how to improve that, but I think it can be improved in a later patch.

I think both features could be useful. For example sometimes a
maintainer might want to know if applying only a patch that is in the
middle of a patch series (for example something security related)
could work without having to apply the previous patchs. So maybe both
a --check and a --dry-run with different behavior regarding patch
series would make sense.

Re: [PATCH] am: add --check option

[René Scharfe]

Am 03.06.19 um 16:25 schrieb Drew DeVault:
> @@ -2195,6 +2206,8 @@ int cmd_am(int argc, const char **argv, const char *prefix)
>  			0, PARSE_OPT_NONEG),
>  		OPT_BOOL('c', "scissors", &state.scissors,
>  			N_("strip everything before a scissors line")),
> +		OPT_BOOL(0, "check", &state.check,
> +			N_("instead of applying the patch, see if the patch is applicable")),
>  		OPT_PASSTHRU_ARGV(0, "whitespace", &state.git_apply_opts, N_("action"),
>  			N_("pass it through git-apply"),
>  			0),
>

Git apply has a --check option as well for the same purpose.  Other
commands have an equivalent option called --dry-run instead.  Would it
make sense to move towards the latter for greater consistency?

René

Re: [PATCH] am: add --check option

[Johannes Sixt]

Am 03.06.19 um 16:25 schrieb Drew DeVault:
> +--check::
> +	Instead of applying the patch(es), see if they are
> +	applicable to the current working tree and/or the index
> +	file and detects errors.

I have to wonder how --check works when 'am' applies multiple patches.

When the second patch in a patch series depends on that the first patch
is fully applied, what does --check do? Without the first patch applied,
then a naive check of the second patch will certainly fail, doesn't it?

-- Hannes

Re: [PATCH] am: add --check option

[SZEDER Gábor]

On Mon, Jun 03, 2019 at 10:25:23AM -0400, Drew DeVault wrote:
> ---
>  Documentation/git-am.txt |  7 ++++++-
>  builtin/am.c             | 13 +++++++++++++
>  2 files changed, 19 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/git-am.txt b/Documentation/git-am.txt
> index fc3b993c33..bc01e87d85 100644
> --- a/Documentation/git-am.txt
> +++ b/Documentation/git-am.txt
> @@ -9,7 +9,7 @@ git-am - Apply a series of patches from a mailbox
>  SYNOPSIS
>  --------
>  [verse]
> -'git am' [--signoff] [--keep] [--[no-]keep-cr] [--[no-]utf8]
> +'git am' [--signoff] [--keep] [--check] [--[no-]keep-cr] [--[no-]utf8]
>  	 [--[no-]3way] [--interactive] [--committer-date-is-author-date]
>  	 [--ignore-date] [--ignore-space-change | --ignore-whitespace]
>  	 [--whitespace=<option>] [-C<n>] [-p<n>] [--directory=<dir>]
> @@ -44,6 +44,11 @@ OPTIONS
>  --keep-non-patch::
>  	Pass `-b` flag to 'git mailinfo' (see linkgit:git-mailinfo[1]).
>  
> +--check::
> +	Instead of applying the patch(es), see if they are
> +	applicable to the current working tree and/or the index
> +	file and detects errors.

Note that in "real" patch series a later patch quite often depends on
the changes made in earlier patches, and this option should somehow
account for that.  I'm not sure how to do that without actually
applying the patches, though...

  # Create two patches, each modifying the same line in the same file.
  $ echo 0 >file
  $ git add file
  $ git commit -m initial
  [master 956965a] initial
   1 file changed, 1 insertion(+)
   create mode 100644 file
  $ echo 1 >file
  $ git commit -m one file
  [master fd65db1] one
   1 file changed, 1 insertion(+), 1 deletion(-)
  $ echo 2 >file
  $ git commit -m two file
  [master 1b878f1] two
   1 file changed, 1 insertion(+), 1 deletion(-)
  $ git format-patch -2
  0001-one.patch
  0002-two.patch

  # This shows that the second patch is applicable on top of the
  # first:
  $ git checkout HEAD^
  HEAD is now at fd65db1 one
  $ git apply --check 0002-two.patch ; echo $?
  0

  # But 'git am --check' reports that the two patches can't be
  # applied on the initial commit, because it attempts to apply the
  # second patch on the initial commit as well, instead on top of the
  # first:
  $ git checkout HEAD^
  Previous HEAD position was fd65db1 one
  HEAD is now at 956965a initial
  $ ~/src/git/bin-wrappers/git am --check 0001-one.patch 0002-two.patch
  Applying: one
  Applying: two
  error: patch failed: file:1
  error: file: patch does not apply
  Patch failed at 0002 two

  # Though, of course, they can be applied just fine:
  $ ~/src/git/bin-wrappers/git am 0001-one.patch 0002-two.patch
  Applying: one
  Applying: two

[PATCH] am: add --check option

[Drew DeVault]

---
 Documentation/git-am.txt |  7 ++++++-
 builtin/am.c             | 13 +++++++++++++
 2 files changed, 19 insertions(+), 1 deletion(-)

diff --git a/Documentation/git-am.txt b/Documentation/git-am.txt
index fc3b993c33..bc01e87d85 100644
--- a/Documentation/git-am.txt
+++ b/Documentation/git-am.txt
@@ -9,7 +9,7 @@ git-am - Apply a series of patches from a mailbox
 SYNOPSIS
 --------
 [verse]
-'git am' [--signoff] [--keep] [--[no-]keep-cr] [--[no-]utf8]
+'git am' [--signoff] [--keep] [--check] [--[no-]keep-cr] [--[no-]utf8]
 	 [--[no-]3way] [--interactive] [--committer-date-is-author-date]
 	 [--ignore-date] [--ignore-space-change | --ignore-whitespace]
 	 [--whitespace=<option>] [-C<n>] [-p<n>] [--directory=<dir>]
@@ -44,6 +44,11 @@ OPTIONS
 --keep-non-patch::
 	Pass `-b` flag to 'git mailinfo' (see linkgit:git-mailinfo[1]).
 
+--check::
+	Instead of applying the patch(es), see if they are
+	applicable to the current working tree and/or the index
+	file and detects errors.
+
 --[no-]keep-cr::
 	With `--keep-cr`, call 'git mailsplit' (see linkgit:git-mailsplit[1])
 	with the same option, to prevent it from stripping CR at the end of
diff --git a/builtin/am.c b/builtin/am.c
index 912d9821b1..9ae90dec28 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -112,6 +112,7 @@ struct am_state {
 	int keep; /* enum keep_type */
 	int message_id;
 	int scissors; /* enum scissors_type */
+	int check;
 	struct argv_array git_apply_opts;
 	const char *resolvemsg;
 	int committer_date_is_author_date;
@@ -1422,6 +1423,8 @@ static int run_apply(const struct am_state *state, const char *index_file)
 	} else
 		apply_state.check_index = 1;
 
+	apply_state.check = state->check;
+
 	/*
 	 * If we are allowed to fall back on 3-way merge, don't give false
 	 * errors during the initial attempt.
@@ -1565,6 +1568,9 @@ static void do_commit(const struct am_state *state)
 	const char *reflog_msg, *author;
 	struct strbuf sb = STRBUF_INIT;
 
+	if (state->check)
+		return;
+
 	if (run_hook_le(NULL, "pre-applypatch", NULL))
 		exit(1);
 
@@ -1775,6 +1781,11 @@ static void am_run(struct am_state *state, int resume)
 			printf_ln(_("Patch failed at %s %.*s"), msgnum(state),
 				linelen(state->msg), state->msg);
 
+			if (state->check) {
+				am_destroy(state);
+				exit(128);
+			}
+
 			if (advice_amworkdir)
 				advise(_("Use 'git am --show-current-patch' to see the failed patch"));
 
@@ -2195,6 +2206,8 @@ int cmd_am(int argc, const char **argv, const char *prefix)
 			0, PARSE_OPT_NONEG),
 		OPT_BOOL('c', "scissors", &state.scissors,
 			N_("strip everything before a scissors line")),
+		OPT_BOOL(0, "check", &state.check,
+			N_("instead of applying the patch, see if the patch is applicable")),
 		OPT_PASSTHRU_ARGV(0, "whitespace", &state.git_apply_opts, N_("action"),
 			N_("pass it through git-apply"),
 			0),
-- 
2.21.0