Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve "git am" support. #854

Open
wants to merge 9 commits into
base: master
from

Conversation

@vathpela
Copy link
Contributor

vathpela commented Sep 23, 2019

This makes a couple of related changes to '%autosetup -S git_am' to make
debugging using the rpm build dir as a git worktree significantly
easier:

  • creates a branch named "rpmbuild" at the initial commit, switches to
    it, and sets the branch's upstream to master so that in the work tree
    things like "git rebase -i" have a default behavior that makes sense
  • introduces the optional _scm_apply_patches%{__scm}, which if defined
    will be called to apply all patches at once. The patches are not
    provided as an argument, %{patches} must be used. This allows a
    developer debugging a package to use "git am --continue" in the
    work tree after one patch fails, so the developer can continue fixing
    all the patches up at once and regenerate the patchset with
    "git format-patch" when they finish.
  • introduces __scm_apply_git_am_options and uses it for both
    %__scm_apply_git_am and %__scm_apply_patches_git_am . By default,
    this is populated with three options:
    --ignore-whitespace
    this makes whitespace discrepancies in diff context be ignored
    --whitespace=nowarn
    this makes whitespace issues in the patch itself, such as trailing
    whitespace, be ignored
    --committer-date-is-author-date
    this makes the date of the commit be the date from the patch, not
    the date rpmbuild is run

Signed-off-by: Peter Jones pjones@redhat.com

@vathpela vathpela force-pushed the vathpela:rpmbuild-git-am-improvements branch from ab637c6 to d2f6f1b Sep 23, 2019
@pmatilai

This comment has been minimized.

Copy link
Contributor

pmatilai commented Sep 25, 2019

Nice to see that at least somebody has discovered this and finding the idea useful enough to improve.

Please split to independent commits though, too much unrelated change packed into one here:

  • creating a branch where patches are applied is one change and should arguably be done for all scm's that meaningfully support it
  • support for options in apply should also be a general thing, not limited to git am, and adding a mechanism generally should be separated from adding particular defaults
  • I do see the point with git am --continue but short-cutting to %{patches} breaks other use-cases like applying ranges of patches. Need to find a way to do this without breaking others.
@vathpela

This comment has been minimized.

Copy link
Contributor Author

vathpela commented Sep 25, 2019

Nice to see that at least somebody has discovered this and finding the idea useful enough to improve.

Please split to independent commits though, too much unrelated change packed into one here:

Sure, that's fine by me.

* creating a branch where patches are applied is one change and should arguably be done for all scm's that meaningfully support it

I see your point, and I agree with it, but I have a concern: I wouldn't know where to begin for any of the other scm systems, so I'm not writing support for all of them. That said, I don't see that there's much generalization to be had here (in terms of the code); it appears they'll all have to be separate implementations entirely. It doesn't seem like there's much good reason to gate one scm doing this on support in the others, so I'll send this as its own patch for git.

* support for options in apply should also be a general thing, not limited to git am, and adding a mechanism generally should be separated from adding particular defaults

Sure - but again I'm only comfortable implementing that for git. Tell me what /sort/ of mechanism are you looking for, and I'll happily write it for all the git cases, and write the general hook for it so others can implement it for the other scm systems. Are you okay with the approach I took with seeing if %{expand:%%{?scm_apply%{_scm}_options}} exists as the method for generalizing it?

* I do see the point with `git am --continue` but short-cutting to %{patches} breaks other use-cases like applying ranges of patches. Need to find a way to do this without breaking others.

Would you be prefer something like a global that can be defined to control this (a la disabling debuginfo), or would you prefer an argument to %autosetup, or some other method I haven't thought of?

This changes the git autosetup handlers so that they do the initial
commit of the expanded tarball on "master", then switch to a branch
"rpm-build" before applying patches.  Additionally it sets the
"rpm-build" branch's upstream to "master", so that in the active work
tree where the "rpm-build" is checked out, commands such as
"git rebase -i" automatically have a default behavior that makes sense.

Signed-off-by: Peter Jones <pjones@redhat.com>
@vathpela vathpela force-pushed the vathpela:rpmbuild-git-am-improvements branch from d2f6f1b to 5d3fe63 Sep 25, 2019
@vathpela

This comment has been minimized.

Copy link
Contributor Author

vathpela commented Sep 25, 2019

In anticipation of your responses above, I've reworked this patch series. If you'd like it differently than I guessed, let me know.

@vathpela vathpela force-pushed the vathpela:rpmbuild-git-am-improvements branch from 5d3fe63 to 81594aa Sep 25, 2019
@Conan-Kudo

This comment has been minimized.

Copy link
Member

Conan-Kudo commented Sep 26, 2019

This looks very cool! I've not had a chance to look at it in-depth, but the first pass looks pretty good!

vathpela added 6 commits Sep 25, 2019
This adds a general mechanism for each scm system so that in rpm we can
add:

%_default_FOOSCM_apply_flags --old-school-apply-feature
%_default_FOOSCM_import_flags --old-school-import-feature
%_default_FOOSCM_commit_flags --old-school-commit-feature

and they'll get used by %autosetup/%setup/%apply_patch/etc
automatically, but if the user defines either or both of:

%__scm_FOOSCM_apply_flags --fancy-new-apply-feature
%__scm_FOOSCM_import_flags --fancy-new-import-feature
%__scm_FOOSCM_commit_flags --fancy-new-commit-feature

They will override the defaults.  This also moves the definitions for
%_default_patch_fuzz and %_default_patch_flags into the SCM section of
macros.in, and preserves the existing behavior in cases where those
macros are redefined.

Note that this continues in the spirit of how %_default_patch_flags
works, on the assumption that we always need some flags but others may
be a more dependent on a packager's preferences and workflow.  With that
in mind, this patch does not make any attempt to move the usage of any
of the existing flags that are currently in use into any scm's default
flags - right now the defaults are exactly as they were before, and only
on scm systems that wind up using %_default_patch_flags are
%_apply_flags currently meaningful without the user defining
%__scm_FOOSCM_apply_flags.

One minor exception: this makes %_scm_bzr_apply()'s invocation of
%__patch use %_default_patch_flags, which appears to be what it should
have done before.  It does so via the expansion of %_apply_flags ->
%_defualt_bzr_apply_flags -> %_default_patch_apply_flags, so setting
%__scm_patch_apply_flags or %__scm_apply_bzr_flags will alter its
behavior, with the latter overriding the former.

[v2: made the naming less crazy so I could get it right]

Signed-off-by: Peter Jones <pjones@redhat.com>
This defines the following macros:

_default_git_apply_flags
_default_git_import_flags
_default_git_am_apply_flags
_default_git_am_import_flags

In each of these cases, these are the following git arguments, as
appropriate for which command they're being used in:

  --ignore-whitespace
    this makes whitespace discrepancies in diff context be ignored
  --whitespace=nowarn
    this makes whitespace issues in the patch itself, such as trailing
    whitespace, be ignored
  --committer-date-is-author-date
    this makes the date of the commit be the date from the patch, not
    the date rpmbuild is run

Signed-off-by: Peter Jones <pjones@redhat.com>
This adds optional -m <low patch number> and -M <high patch number>
arguments to %patches and %sources, so that they can be used directly by
macros that use those.

Signed-off-by: Peter Jones <pjones@redhat.com>
This makes it so you can control which patches get applied via
%autosetup and %autopatch with fine grained control using -m and -M on
either or both %autosetup and %autopatch (in combination with -N on
%autosetup, if desired)

Signed-off-by: Peter Jones <pjones@redhat.com>
This makes autopatch use %{__scm_apply_patches_%{__scm}} to apply
multiple patches in a single command if that macro is defined.

Signed-off-by: Peter Jones <pjones@redhat.com>
Signed-off-by: Peter Jones <pjones@redhat.com>
@vathpela vathpela force-pushed the vathpela:rpmbuild-git-am-improvements branch from 81594aa to 319a878 Sep 27, 2019
@vathpela

This comment has been minimized.

Copy link
Contributor Author

vathpela commented Sep 27, 2019

Okay, I think this one should be pretty much fully baked - there's test cases now for %{patches -m N -M N}, %{sources...}, %autosetup -S git_am, %autosetup -S git, each of those with -N, and %autopatch after each of those.

vathpela added 2 commits Sep 26, 2019
Signed-off-by: Peter Jones <pjones@redhat.com>
Signed-off-by: Peter Jones <pjones@redhat.com>
@vathpela vathpela force-pushed the vathpela:rpmbuild-git-am-improvements branch from 319a878 to cc9ee18 Sep 30, 2019
%_default_commit_flags %{expand:%%{?_default_%{__scm}_commit_flags}}

# Choose between default commit flags and user-defined commit flags
%_commit_flags %{expand:%%{!?__scm_%{__scm}_commit_flags:%%{?_default_commit_flags}}%%{?__scm_%{__scm}_commit_flags:%%{?__scm_%{__scm}_commit_flags}}}

This comment has been minimized.

Copy link
@pmatilai

pmatilai Oct 9, 2019

Contributor

%_apply_flags, %_import_flags and %_commit flags are awfully generic names. Please prepend "scm" or such in there someplace, eg %_scm_apply_flags (but I didn't verify whether that clashes with something)

print(s.." ") \
end\
end\
}}

This comment has been minimized.

Copy link
@pmatilai

pmatilai Oct 9, 2019

Contributor

These kinda scream for a common min/max handling function, but might be easier said than done in the macro land.

end\
}\
}\
}

This comment has been minimized.

Copy link
@pmatilai

pmatilai Oct 9, 2019

Contributor

This huge %{expand:...} with all those escaped %'s is a bit unwieldy. %{expand:...} is better limited to the smallest possible scope because those expansions and escapes add up (see eg #313 for context).

Also makes me wonder if it'd be saner to handle this by making __scm_apply_patches_SCM mandatory for all and just point all the other SCM's to the generic %{lua:} bit (call it __scm_apply_patches_gen or whatever).

Dunno, but this construct does make me feel uneasy.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.